-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [M3-8676] - Provide Customer Notice for OS Distro Nearing EOL/EOS #11253
feat: [M3-8676] - Provide Customer Notice for OS Distro Nearing EOL/EOS #11253
Conversation
const [imageDeprecatedText, setImageDeprecatedText] = React.useState< | ||
null | string | ||
>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to consider deriving state from existing state rather than creating new state for this.
For example:
const { data: image } = useImageQuery(
field.value ?? '',
Boolean(field.value)
);
const showImageDeprecatedWarning =
image !== undefined && isImageDeprecated(image);
Using extra state here might be fine, but deriving from the existing form state could be beneficial because it would gracefully handle things like reacting to the form being reset for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you have recommended makes sense but when I implemented it there is a visible delay for the text displayed. The delay is caused because of the call to useImageQuery
. If it is not much of an issue I will push the updated changes
Screen.Recording.2024-11-18.at.1.31.45.PM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point. It is a tradeoff. You can make the final call here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking a little more, I think we should probably do it by deriving for now so that it is constant with with other parts of Linode Create like packages/manager/src/features/Linodes/LinodeCreate/Summary/Summary.tsx
for example.
We can probably make some optimizations on the React Query level if we need to address the small delay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Pushed the changes you've mentioned.
{imageDeprecatedText && ( | ||
<Notice | ||
dataTestId="os-distro-deprecated-image-notice" | ||
spacingBottom={0} | ||
spacingTop={16} | ||
variant="warning" | ||
> | ||
<Typography fontFamily={theme.font.bold}> | ||
{imageDeprecatedText}. After this date, this OS distribution will | ||
no longer receive security updates or technical support. We | ||
recommend selecting a newer supported version to ensure continued | ||
security and stability for your linodes. | ||
</Typography> | ||
</Notice> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having imageDeprecatedText
be a string, what do you think about having a boolean control this, and putting the content directly in the JSX
{imageDeprecatedText && ( | |
<Notice | |
dataTestId="os-distro-deprecated-image-notice" | |
spacingBottom={0} | |
spacingTop={16} | |
variant="warning" | |
> | |
<Typography fontFamily={theme.font.bold}> | |
{imageDeprecatedText}. After this date, this OS distribution will | |
no longer receive security updates or technical support. We | |
recommend selecting a newer supported version to ensure continued | |
security and stability for your linodes. | |
</Typography> | |
</Notice> | |
)} | |
{image && showImageDeprecatedWarning && ( | |
<Notice | |
dataTestId="os-distro-deprecated-image-notice" | |
spacingBottom={0} | |
spacingTop={16} | |
variant="warning" | |
> | |
<Typography fontFamily={(theme) => theme.font.bold}> | |
{image.label} will reach its end-of-life on{' '} | |
{formatDate(image.eol ?? '', { format: 'MM/dd/yyyy' })}. After | |
this date, this OS distribution will no longer receive security | |
updates or technical support. We recommend selecting a newer | |
supported version to ensure continued security and stability for | |
your linodes. | |
</Typography> | |
</Notice> | |
)} |
I find that this is a bit cleaner and more readable.
@@ -42,6 +49,16 @@ export const OperatingSystems = () => { | |||
const onChange = async (image: Image | null) => { | |||
field.onChange(image?.id ?? null); | |||
|
|||
let deprecatedNoticeText = null; | |||
if (image && isImageDeprecated(image)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh beat me to it
A couple comments on the copy, I believe we should take these concerns to UX: Adding a qualifierIMO, the phrase "The selected Ubuntu 23.10 will reach ..." doesn't seem valid. It should be qualified, for example "The selected OS distribution, Ubuntu 23.10, will reach ...". Past EOL datesWe should ask UX for a slight variation of the copy when the EOL date is in the past. As a suggestion we could say:
|
I've pushed the changes to ensure the tense matches with the eol date. |
Coverage Report: ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for responding to feedback!
@@ -68,6 +79,34 @@ export const OperatingSystems = () => { | |||
value={field.value} | |||
variant="public" | |||
/> | |||
{image && showImageDeprecatedWarning && ( | |||
<Notice | |||
dataTestId="os-distro-deprecated-image-notice" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this dataTestId
if we don't have a unit test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harsh-akamai & @bnussman-akamai
I don't fully understand why we're doing an extra image query - why not displaying this within ImageSelect so:
- every consumer gets the warning
- we use the images query we already have running and use an effect in the component to conditionally display the warning
Please let me know if there is a good reason not to do this and I'll remove my request (wanted to prevent this to be merged if can be improved)
it's worth noticing the lag with the notice display, especially on a slower connection
Doing this within The only "downside" I can think of moving it in there would be: If we want to keep this as-is, I could push a change on the React Query level to alleviate the extra fetch |
I'd like to specify the ticket requirement, really - are we to add this notice on all flows? cause if so it's a no brainer |
@abailly-akamai Yeah, you make a good point. This would be really nice to have in other flows. |
@abailly-akamai The ticket specifically mentions to add the notice in the Linodes/Create screen. |
<ImageSelect | ||
textFieldProps={{ | ||
required: true, | ||
tooltipText: | ||
'Select which images are compatible with this StackScript. "Any/All" allows you to use private images.', | ||
}} | ||
anyAllOption | ||
data-qa-stackscript-target-select | ||
disabled={disabled} | ||
errorText={hasErrorFor('images')} | ||
label="Target Images" | ||
multiple | ||
onChange={onSelectChange} | ||
placeholder="Select image(s)" | ||
value={selectedImages} | ||
variant="public" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shifted ImageSelect
out of the grid so that the banner could cover full width
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cloud Manager UI test results🎉 469 passing tests on test run #15 ↗︎
|
Cloud Manager E2E Run #6953
Run Properties:
|
Project |
Cloud Manager E2E
|
Branch Review |
develop
|
Run status |
Passed #6953
|
Run duration | 30m 01s |
Commit |
4afffcb742: feat: [M3-8676] - Provide Customer Notice for OS Distro Nearing EOL/EOS (#11253)
|
Committer | Harsh Shankar Rao |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
4
|
Pending |
2
|
Skipped |
0
|
Passing |
469
|
View all changes introduced in this branch ↗︎ |
Description 📝
Add Notice for OS Distro Nearing EOL/EOS
Changes 🔄
Target release date 🗓️
10/12
Preview 📷
How to test 🧪
Verification steps
Preview
As an Author, I have considered 🤔
As an Author, before moving this PR from Draft to Open, I confirmed ✅