Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(design-system): add dropdown menu component and stories #2522

Merged
merged 36 commits into from
Oct 20, 2021
Merged

feat(design-system): add dropdown menu component and stories #2522

merged 36 commits into from
Oct 20, 2021

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Aug 3, 2021

Features:

Before & After Screenshots

In storybook

@tshuli tshuli changed the base branch from form-v2/develop to form-v2/inlinemsg-component August 3, 2021 13:32
seaerchin
seaerchin reviewed Aug 4, 2021
View reviewed changes
frontend/src/components/Menu/DropdownMenu.stories.tsx Outdated Show resolved Hide resolved
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
Base automatically changed from form-v2/inlinemsg-component to form-v2/develop August 4, 2021 09:21
@tshuli tshuli force-pushed the form-v2/menu-component branch 3 times, most recently from ea158ff to d3fecd3 Compare August 6, 2021 06:56
@tshuli tshuli requested a review from seaerchin August 11, 2021 03:22
seaerchin
seaerchin reviewed Aug 11, 2021
View reviewed changes
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

i think there's a slight error on behaviour - both the menu button + options "shrink" when you hover over them. i think this is because there's a change in border size, which leads to the component resizing.

frontend/src/components/Menu/DropdownMenu.stories.tsx Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
@seaerchin
Copy link
Contributor

seaerchin commented Aug 11, 2021 •
edited
Loading

also, i can't seem to tab target the options + options have border on _hover, which seems to differ from the figma

@tshuli tshuli force-pushed the form-v2/menu-component branch from 004c718 to fd6b183 Compare August 12, 2021 08:25
@tshuli
Copy link
Contributor Author

tshuli commented Aug 12, 2021

for options, they can be selected using arrow keys instead of tab - as per aria specs

also fixed the css for options _hover

@tshuli
Copy link
Contributor Author

tshuli commented Aug 16, 2021 •
edited
Loading

Note that design has clarified that theming for dropdown menu is different from dropdown field (see form design system)

@tshuli tshuli force-pushed the form-v2/menu-component branch from fd6b183 to 941d7a1 Compare September 1, 2021 04:09
@tshuli tshuli requested a review from karrui September 1, 2021 09:05
karrui
karrui requested changes Sep 1, 2021
View reviewed changes
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
@tshuli
Copy link
Contributor Author

tshuli commented Sep 2, 2021

Thanks both! I've edited it to use the existing Button theming

@tshuli tshuli requested review from karrui and mantariksh September 2, 2021 13:12
karrui
karrui requested changes Sep 6, 2021
View reviewed changes
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/theme/components/DropdownMenu.ts Outdated Show resolved Hide resolved
frontend/src/components/Menu/DropdownMenu.tsx Outdated Show resolved Hide resolved
frontend/src/components/Menu/DropdownMenu.stories.tsx Outdated Show resolved Hide resolved
@tshuli tshuli requested a review from karrui September 6, 2021 08:50
@tshuli tshuli force-pushed the form-v2/menu-component branch 2 times, most recently from 99204e7 to 9f74cce Compare September 6, 2021 08:53
@tshuli
Copy link
Contributor Author

tshuli commented Sep 8, 2021

@karrui have confirmed the designs with pearly, for your re-review!

karrui
karrui approved these changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@karrui karrui left a comment •
edited
Loading

Choose a reason for hiding this comment

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

some styling still looks off, but i'll leave that to @pearlyong, codewide looks fine

edit: might need to merge in form-v2/develop into this branch, seems to have some leftover ci/cd conflicts

@tshuli tshuli requested a review from pearlyong September 9, 2021 02:25
@tshuli tshuli force-pushed the form-v2/menu-component branch from 9f74cce to 24d74c2 Compare September 9, 2021 02:26
mantariksh
mantariksh reviewed Sep 16, 2021
View reviewed changes
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

can we also stick to a single naming convention pls - either Menu or DropdownMenu? it's confusing that everything is named after the parent folder except this. (noticed because I'm using this for Datepicker haha)

@tshuli tshuli force-pushed the form-v2/menu-component branch from 24d74c2 to 9e8e632 Compare September 21, 2021 05:55
tshuli added 21 commits September 30, 2021 14:06
@tshuli
style: add more css for options
0608480
@tshuli
style: change px to rem
0943e7d
@tshuli
refactor: use theme override
32e2327
@tshuli
style: fix menu stretch space-between
2789475
@tshuli
chore: remove argType control
1d3fa1b
@tshuli
refactor: isStretch and isClear props
38217c6
@tshuli
refactor: simplify and export as subcomponents
4d19309
@tshuli
refactor: using existing button themes
9e2bee3
@tshuli
chore: update exports
6e20990
@tshuli
chore: shift props destructure to end
10f3590
@tshuli
style: use boxshadow instead of border
b2738b5
@tshuli
style: retain focus border on active
47250cf
@tshuli
style: remove border radius
966e74b
@tshuli
style: use outline instead of border
b3b766e
@tshuli
chore: simplify naming
3410b6e
@tshuli
fix: boxshadow on active to show only for outline variant
2ae9c7f
@tshuli
style: retain background on focus
bb3fffd
@tshuli
style: remove extra padding
9f00408
@tshuli
chore: rename dropdownmenu to menu throughout
fa15e18
@tshuli
style: add darker font and border for hover state
293e57e
@tshuli
style: darker border on hover only for outline variant
8ff96a9
@tshuli tshuli force-pushed the form-v2/menu-component branch from 8f77dc4 to 8ff96a9 Compare September 30, 2021 06:07
karrui
karrui approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm with nits

frontend/src/components/Menu/Menu.tsx Outdated Show resolved Hide resolved
frontend/src/components/Menu/Menu.tsx Outdated Show resolved Hide resolved
@tshuli tshuli merged commit 2643776 into form-v2/develop Oct 20, 2021
@tshuli tshuli deleted the form-v2/menu-component branch October 20, 2021 07:23
@karrui karrui mentioned this pull request Dec 20, 2021
Closed
This was referenced Oct 4, 2022
Closed
Merged
@justynoh justynoh mentioned this pull request Oct 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seaerchin seaerchin seaerchin left review comments

@mantariksh mantariksh mantariksh left review comments

@karrui karrui karrui approved these changes

@pearlyong pearlyong Awaiting requested review from pearlyong

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tshuli @seaerchin @karrui @mantariksh