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.

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(odd): menuList and MenuItem ODD support #12511

Merged
merged 6 commits into from
Apr 19, 2023

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Apr 17, 2023

closes RAUT-403

Overview

Change MenuList and MenuItem to be used as the overflow menu in ODD

Screen.Recording.2023-04-18.at.3.40.44.PM.mov

Test Plan

Review the storybooks for MenuList and MenuItem and review the menu buttons in the desktop app to make sure they retain correct UI and review the Navigation menu in the ODD to make sure it has correct Ui.

Changelog

  • Change MenuItem to take in isAlert prop and use media query, modify storybook and test
  • Change MenuList to take in isOnDevice and OnClick props, change button prop to be children and fix all usages of it in the desktop (it was only used in Module overflow menu). Modify storybook and test
  • Change NavigationMenu to use MenuItem and MenuList
  • add ODD viewport to storybook/preview

Review requests

  • see test plan

Risk assessment

low

@jerader jerader requested a review from a team as a code owner April 17, 2023 20:29
@jerader jerader requested review from koji, ewagoner and a team and removed request for a team April 17, 2023 20:29
@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #12511 (63a7265) into edge (4f3042b) will increase coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12511      +/-   ##
==========================================
+ Coverage   73.43%   73.70%   +0.27%     
==========================================
  Files        1498     2255     +757     
  Lines       49066    61974   +12908     
  Branches     2991     6581    +3590     
==========================================
+ Hits        36030    45678    +9648     
- Misses      12580    14733    +2153     
- Partials      456     1563    +1107     
Flag Coverage Δ
app 72.21% <100.00%> (+25.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...DeviceDisplay/ProtocolDashboard/LongPressModal.tsx 52.00% <ø> (ø)
app/src/atoms/MenuList/MenuItem.tsx 100.00% <100.00%> (ø)
app/src/atoms/MenuList/index.tsx 100.00% <100.00%> (ø)
...pp/src/organisms/ModuleCard/ModuleOverflowMenu.tsx 100.00% <100.00%> (ø)
...isms/OnDeviceDisplay/Navigation/NavigationMenu.tsx 100.00% <100.00%> (ø)

... and 753 files with indirect coverage changes

color: ${COLORS.darkBlackEnabled};
padding: ${SPACING.spacing3} 0.75rem ${SPACING.spacing3} 0.75rem;
export const MenuItem: BtnComponent = styled.button<ButtonProps>`
align-items: ${props => (props.isOnDevice ? ALIGN_CENTER : 'auto')};
Copy link
Contributor

@koji koji Apr 18, 2023

Choose a reason for hiding this comment

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

maybe

Suggested change
align-items: ${props => (props.isOnDevice ? ALIGN_CENTER : 'auto')};
align-items: ${({isOnDevice}) => (isOnDevice ? ALIGN_CENTER : 'auto')};

background-color: ${COLORS.transparent};
color: ${COLORS.black}${COLORS.opacity50HexCode};
}

@media ${RESPONSIVENESS.touchscreenMediaQuerySpecs} {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no idea how to get hover:none on storybook, i guess to test this in storybook, you have to change this const to remove the (hover:none)

@jerader jerader requested a review from koji April 18, 2023 16:40
@jerader jerader requested a review from a team April 18, 2023 19:43
@ewagoner
Copy link
Contributor

@ewagoner
Copy link
Contributor

Maybe I had something cached somewhere. Things are looking better for me now.
Screenshot 2023-04-19 at 10 52 59 AM

Copy link
Contributor

@ewagoner ewagoner left a comment

Choose a reason for hiding this comment

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

lgtm! Can you make a ticket to convert app/src/pages/OnDeviceDisplay/ProtocolDashboard/LongPressModal.tsx to use these components?

@jerader
Copy link
Collaborator Author

jerader commented Apr 19, 2023

lgtm! Can you make a ticket to convert app/src/pages/OnDeviceDisplay/ProtocolDashboard/LongPressModal.tsx to use these components?

@ewagoner, i went ahead and changed LongPressModal to use these since it is a small lift 😄

@ewagoner
Copy link
Contributor

@ewagoner, i went ahead and changed LongPressModal to use these since it is a small lift 😄

Wow -- you are on it! Looks great on my dev kit. Thanks!

@jerader jerader merged commit 066e13e into edge Apr 19, 2023
@jerader jerader deleted the odd_app-change-menu-item-for-odd branch April 19, 2023 16:12
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.

3 participants