-
Notifications
You must be signed in to change notification settings - Fork 365
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
refactor/upcoming: [M3-8116] - Event Messages - Part 1 #10517
Conversation
packages/api-v4/src/account/types.ts
Outdated
'vpc_update', | ||
] as const; | ||
|
||
export type EventAction = typeof EventActionValues[number]; |
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.
- No change affecting the types (only some added from fetching the hosting Db for
events
- Exporting
EventActionValues
allows us to iterate over a list of values for our tests, ensuring we have a corresponding message for each action.
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 adding some of the missing ones. I'd noticed firewall_apply
recently was lacking an event message, and that'll be fixed here.
</Snackbar> | ||
); | ||
}, | ||
}; |
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.
Adding an example on how to fetch a single event for a snackbar
const link = getLinkForEvent(event.action, entity); | ||
|
||
return link ? <Link to={link}>{entity.label}</Link> : renderDefault(); | ||
}; |
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.
Instead of parsing and formatting event, we compose them with reusable components
display: 'inline', | ||
fontSize: '0.75rem', | ||
padding: '0.15rem 0.25rem', | ||
}); |
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.
Instead of parsing and formatting event, we compose them with reusable components
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.
replaced with the v2 version. This did not have much value as it was.
}, | ||
width: '60%', | ||
width: 'calc(100% - 400px)', |
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.
improves /event table row layout in both v1 and v2 (very slight changes)
<Hidden mdDown> | ||
<StyledTableCell data-qa-events-time-header> | ||
<StyledTableCell data-qa-events-time-header sx={{ width: 150 }}> |
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.
Implementing approved events table UX changes
- remove avatar from table
- add a user column
- improve responsive handling
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.
This file was not in use at all 🧹
</> | ||
) : 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.
New version contains approved UX changes
- User name is not appended to message, and added next to the timestamp, separated by a pipe
- Gravatar size is slightly decreased
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.
@@ -235,7 +235,6 @@ const StyledNotificationItem = styled(Box, { | |||
shouldForwardProp: omittedProps(['header']), | |||
})<{ header: string }>(({ theme, ...props }) => ({ | |||
'& p': { | |||
color: theme.textColors.headlineStatic, |
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.
removing to make messages the same color as regular copy (not sure why it was overridden)
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Thanks for all the suggestions and the thorough review, @mjac0bs! I know it's a lot to read through! Updated PR with all copy and type suggestions. |
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.
This is such an awesome improvement! Thanks @abailly-akamai.
packages/manager/src/features/NotificationCenter/NotificationData/RenderEvent.tsx
Outdated
Show resolved
Hide resolved
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.
Confirmed:
- Updated Storybook stories load
- Test coverage is added for the changes and tests pass
- The edge case for no usernames with the | or "by " showing up was addressed
Suggested:
- A lot of minor nits in event message factories. Some typos (that probably existed before this PR), the need for a couple periods, some places where we missed bolding and might have missed linking an entity.
- The need to fix a contrast issue with pre code block styling in dark mode.
- The need to shrink the gravatar size for in-progress events.
Pending the resolution of my 19 or so comments (😅 ), I'm approving this because I've been through all 71 files. Let me know if you want me to circle back to confirm any fixes that are implemented. Thanks, @abailly-akamai!
</> | ||
) : 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.
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Thanks @mjac0bs and @dwiley-akamai - all feedback addressed (either committed from your suggestion or committed myself). Feedback much appreciated! |
@abailly-akamai There are some hidden comments that github collapsed (which I wish it wouldn't do), that I'm seeing neither marked as Outdated or Resolved. Did those get missed by accident? |
@mjac0bs oops! will certainly address those, thx for catching! |
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Description 📝
This PR is part 1 of several contributions to refactor the way we are rendering event messages to our users.
It does not currently affect production. It is behind a feature flag, and once merged meant to sit in develop for some time in order to catch potential formatting issues.
The goals behind the refactor are rather simple:
Changes 🔄
eventMessagesV2
feature flag/events
+/linodes/{linodeId}/activity
)For more details about changes, see self review and screenshots
Target release date 🗓️
Preview 📷
Include a screenshot or screen recording of the change
💡 Use
<video src="" />
tag when including recordings in table.How to test 🧪
Prerequisites
eventMessagesV2
flagVerification steps
Cloud Manager
/events
page/linodes/{linodeId}/activity
Storybook
Docs
yarn docs
As an Author I have considered 🤔
Check all that apply