v3: Menu is using ListItem #1295
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gretanausedaite
requested review from
a team,
mayank99 and
adamhock
and removed request for
a team
This comment was marked as resolved.
This comment was marked as resolved.
mayank99
reviewed
May 24, 2023
mayank99
reviewed
May 25, 2023
mayank99
reviewed
May 25, 2023
| ); | ||
|
|
||
| return React.cloneElement<MenuItemProps>(menuItem, { |
There was a problem hiding this comment.
the original reason for using cloneElement here was for adding extra stuff to menuItem returned by custom itemRenderer.
so if we're removing it, there needs to be a way to expose equivalent functionality
There was a problem hiding this comment.
I was thinking if we need to remove this cloneElement statement?
Reverted changes for now.
There was a problem hiding this comment.
it's ok to keep for now because we are not passing className inside it. we can try to figure out a better solution in the future
mayank99
reviewed
May 30, 2023
mayank99
reviewed
May 31, 2023
Co-authored-by: Mayank <[email protected]>
mayank99
reviewed
May 31, 2023
mayank99
reviewed
May 31, 2023
mayank99
reviewed
Jun 1, 2023
mayank99
reviewed
Jun 1, 2023
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Using
ListIteminside ofMenuItem;Updated code to use new MenuItem inside ComboBox and Select;
Updated CSS tests for Menu;
Renamed MenuItem props:
MenuItemSkeleton is still using some menu-item class stuff;
TODO:
Removed some of the cloneObject code
Testing
Visual tests pass;
Updated unit tests for new classnames;
Docs
https://github.com/iTwin/iTwinUI/wiki/iTwinUI-react-v3-migration-guide#menuitem