Skip to content

feat: add a divider after Account menu item #1927

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

Merged
merged 8 commits into from
Jun 2, 2022

Conversation

AbhineetJain
Copy link
Contributor

@AbhineetJain AbhineetJain commented May 31, 2022

This PR adds a divider after the Account menu item in the User dropdown.

Subtasks

  • extract UserDropdownContent from UserDropdown: This is done to enable better UI testing via Storybook, separating the content displayed when the menu is open and closed.
  • add a divider after the Account menu item
  • update unit tests and stories for the new component

Fixes #1520

Screenshots

Old

Screen Shot 2022-05-31 at 1 26 14 PM

New

Screen Shot 2022-05-31 at 12 41 00 PM

@AbhineetJain
Copy link
Contributor Author

AbhineetJain commented May 31, 2022

I didn't find any Jest tests that failed, do we have any snapshot tests for our React components, or any other sort of screenshots that need an update? The Storybook for UserDropdown does not contain the menu items, may be we should fix that?

@AbhineetJain AbhineetJain marked this pull request as ready for review May 31, 2022 17:25
@AbhineetJain AbhineetJain requested a review from a team as a code owner May 31, 2022 17:25
Copy link
Member

@Kira-Pilot Kira-Pilot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!!!

@Kira-Pilot
Copy link
Member

I didn't find any Jest tests that failed, do we have any snapshot tests for our React components, or any other sort of screenshots that need an update? The Storybook for UserDropdown does not contain the menu items, may be we should fix that?

That's a good callout. I think adding a story with the UserDropdown open would be helpful. A quick way to do this might be to add a prop to open the dropdown, and then set that prop to true in the story. Haven't dug into this very much so lmk if you want to pair!

@greyscaled
Copy link
Contributor

@AbhineetJain Thanks this is great - as are your callouts. Would you be down to add a story for this one?

I think it could be the case that our snapshot is always picking up the menu when it's in a closed state.

@greyscaled
Copy link
Contributor

greyscaled commented May 31, 2022

I didn't find any Jest tests that failed, do we have any snapshot tests for our React components, or any other sort of screenshots that need an update? The Storybook for UserDropdown does not contain the menu items, may be we should fix that?

We don't snapshot test with jest - we use Storybook & Chromatic.

We'd see if any snapshots changed or were broken in CI.


Edit:

Screenshot

chromatic

@AbhineetJain
Copy link
Contributor Author

@Kira-Pilot @vapurrmaid Thanks for the suggestions. Let me add a new story with the dropdown open.

@AbhineetJain AbhineetJain force-pushed the abhineetjain/1520-separate-account-menu-item branch 2 times, most recently from 31c9145 to 1caefa5 Compare June 1, 2022 20:23
@AbhineetJain AbhineetJain force-pushed the abhineetjain/1520-separate-account-menu-item branch from 1caefa5 to 926c6ff Compare June 2, 2022 14:38
@AbhineetJain AbhineetJain merged commit 47c7eda into main Jun 2, 2022
@AbhineetJain AbhineetJain deleted the abhineetjain/1520-separate-account-menu-item branch June 2, 2022 15:09
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
* add a divider after Account menu item

* test: improve Storybook tests

* add closed and open userdropdown tests

* add default isOpen

* extract UserDropdownContent into a single component

* remove the isOpen prop

* address nit comments

* update test name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate Account from Actions in User Menu
3 participants