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

[No QA] DRY up menu item styles #2232

Merged
merged 3 commits into from
Apr 7, 2021
Merged

[No QA] DRY up menu item styles #2232

merged 3 commits into from
Apr 7, 2021

Conversation

roryabraham
Copy link
Contributor

Details

Cleaning up and DRYing some reusable styles between the ReportActionContextMenu and other menu components.

Fixed Issues

No issue, just cleanup.

Tests

Test all the popover / create menus we have for regressions (and buttons should have different styles when hovered or pressed):

  1. ReportActionContextMenu
  2. global create menu
  3. Video chat popover menu
  4. Settings menu

QA Steps

QA will be covered by regular regression testing.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

image

image

Mobile Web

Desktop

iOS

image

image

image

Android

@roryabraham roryabraham requested a review from marcaaron April 5, 2021 21:08
@roryabraham roryabraham self-assigned this Apr 5, 2021
@roryabraham roryabraham requested a review from a team as a code owner April 5, 2021 21:08
@MelvinBot MelvinBot requested review from tgolen and removed request for a team and tgolen April 5, 2021 21:08
@shawnborton
Copy link
Contributor

For this menu here:
image

I wonder if it should actually use the same styles as other bottomDocked menus, like this one?
image

I see that the styles look more compact on desktop when you right-click, and that's where the compact bottomDocked styles are coming from... but maybe we don't need to make the right-click menu look compact? Basically I love the idea of trying to eliminate an extra menu style if we can.

@roryabraham
Copy link
Contributor Author

roryabraham commented Apr 5, 2021

Yeah, so we could reuse the MenuItem in the ReportActionContextMenuItem, and this is what that would look like @shawnborton:

Compact
Same as other popovers

I personally think that the non-compact version takes up too much real estate on the screen and doesn't look as good. What do you think? The "Same as other popovers" version would simplify and DRY up the code some more too (and give us parity one the BottomDocked modals too), so that's good.

@shawnborton
Copy link
Contributor

I think the "same as other popovers" version looks great. It is such a small difference that I don't think it's worth having to maintain two different styles for compact versus normal.

@roryabraham
Copy link
Contributor Author

Okay, I thought it looked just a bit too intense but if you think it looks good I'll make that change!

@marcaaron marcaaron removed their request for review April 6, 2021 22:24
@marcaaron
Copy link
Contributor

Gonna unassign myself from this one so I can focus on some other stuff and not sure it needs an extra review from me :)

@roryabraham
Copy link
Contributor Author

Cool, going to :shipit: then

@roryabraham roryabraham merged commit be7ef5d into master Apr 7, 2021
@roryabraham roryabraham deleted the Rory-DRYMenuItem branch April 7, 2021 06:11
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.

4 participants