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

[core] feat: add MenuItem small modifier, extend MenuDivider width #6269

Merged
merged 26 commits into from
Jul 12, 2023
Merged

[core] feat: add MenuItem small modifier, extend MenuDivider width #6269

merged 26 commits into from
Jul 12, 2023

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Jul 10, 2023 •
edited
Loading

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

  • feat(MenuItem): add support for small size modifier prop
  • feat(MenuItem): remove constraint that prevents both selected "tick" icon and icon prop from being shown at the same time
  • fix(MenuItem): "selected" styles no longer extend "active" styles (this is mostly an internal change)

Reviewers should focus on:

No regressions in selected styling and possible CSS overrides consumers may be applying on .bp5-menu-item.bp5-selected elements.

Screenshot

2023-07-10 12 16 07

@adidahiya adidahiya requested a review from CPerinet July 10, 2023 16:16
@adidahiya adidahiya mentioned this pull request Jul 10, 2023
Closed
@adidahiya
Copy link
Contributor Author

Merge remote-tracking branch 'origin/develop' into ad/menu-item-styling

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

Remove MenuSection and noPadding prop

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

There's a bug in the latest docs preview where the docs navigator menu items have an unexpected left padding:
image

@adidahiya
Copy link
Contributor Author

Also, it looks like <Suggest> items are not getting the "selected" tick icon:
image

@adidahiya
Copy link
Contributor Author

Fix Omnibar, Navigator, and select examples

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

adidahiya
adidahiya commented Jul 12, 2023
View reviewed changes
Copy link
Contributor Author

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

self-review

packages/core/src/components/menu/_menu.scss Outdated Show resolved Hide resolved
packages/core/src/components/menu/menuItem.tsx Outdated Show resolved Hide resolved
packages/docs-app/src/examples/core-examples/menuItemExample.tsx Outdated Show resolved Hide resolved
packages/docs-app/src/examples/select-examples/suggestExample.tsx Outdated Show resolved Hide resolved
@adidahiya
Copy link
Contributor Author

The click-and-hold active state style has changed:

2023-07-12 10 37 23

this is a regression from existing behavior:

2023-07-12 10 38 02

@adidahiya
Copy link
Contributor Author

address self-review, update docs

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

There's also a regression in dark theme styles with the bp5-active modifier class:

image

it's supposed to look like this (from last release):

image

I'll push a commit to fix this regression and the one I noted in my last comment.

adidahiya
adidahiya commented Jul 12, 2023
View reviewed changes
Copy link
Contributor Author

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@adidahiya
Copy link
Contributor Author

fix :active styles

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya adidahiya merged commit f0a9b09 into develop Jul 12, 2023
@adidahiya adidahiya deleted the ad/menu-item-styling branch July 12, 2023 18:12
@chaejunlee chaejunlee mentioned this pull request May 3, 2024
Closed
2 tasks
@evansjohnson evansjohnson mentioned this pull request May 15, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CPerinet CPerinet CPerinet approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@adidahiya @CPerinet