feat(pinned items): Issue #6229 - Adds pinned functionality to List Component #6249
Conversation
…P working for button clicks only.
feat(pinned items): Issue grommet#6229 Adds pinned functionality to List Component
|
It looks like importing and using the icon from grommet-icons has pushed the bundle size over the limit of 132kb gzipped that it's being checked by CircleCI. What is the recommended approach for using an icon? Or is there a better way to accomplish this type of styling. |
The icon needs to be imported from the particular folder within the grommet-icons package, to avoid pulling in all icons just to dereference one of them. For example: |
fix(list): Fixed icon import and typescript definition
feat(pinned): Changes based on PR feedback
chore(merge): Pull latest from Grommet master
feat(pinned): Remove unnecessary Box component
|
@ericsoderberghp Just checking if there is anything else needed on my end for this PR to be merged. |
* refactor(list): Updates to pinned items based on PR feedback * feat(list): Using new FormPin icon. Upgraded grommet-icon version and snapshots.
|
@ericsoderberghp, just following up on this PR to see if there are any additional changes that need to be made before it can be merged in. Thanks! |
Sorry for the delay on this. The code is looking good. Can you resolve the conflicts with package.json and yarn.lock? I think we're good to merge after that. |
* refactor(list): Updates to pinned items based on PR feedback * feat(list): Using new FormPin icon. Upgraded grommet-icon version and snapshots. * chore(deps): Resolving yarn.lock and package.json deps
|
It still looks like there are some conflicts with the master branch for package.json and yarn.lock |
|
Hey Eric. Yea I was just trying to resolve that now. I'm not getting this conflict locally so I'm going to use the Github's Resolve Conflicts screen unless you know of a better way to resolve them. |
I usually re-fetch/pull the master branch and merge via the command line. |
|
Yea that's what I tried. On my local master branch I ran |
|
Ah pulling upstream looks like I was able to get the merge conflict to happen. I'll have this fixed shortly. |
|
@ericsoderberghp, okay I think that should be fixed now |
What does this PR do?
Adds functionality to provide a list of ItemKeys to be pinned in place within the list and will not change when the list is reordered.
Where should the reviewer start?
Please review the context provided in issue 6229 explaining why this feature was requested and the proposed change that was implemented. Then review Storybook and Jest tests for further context regarding functionality and implementation.
Finally a useMemo hook is used to derive the orderableItems from the pinnedItems which is used to replace the data object and the orderableIndex is found for each item. These replace the previously used data and index to find and set active items and updates from within the reorder function.
What testing has been done on this PR?
Tests have been written in Jest as well as a Storybook story. Tests verify that the pinned items are styled appropriately. Also, after a re-orderable item is moved that the pinned item does not change its position.
How should this be manually tested?
Do Jest tests follow these best practices?
screenis used for querying.userEventis used in place offireEvent.asFragment()is used for snapshot testing.Any background context you want to provide?
Pleas see the context provided in Issue 6229.
What are the relevant issues?
Issue #6229
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Yes
Should this PR be mentioned in the release notes?
Yes
Is this change backwards compatible or is it a breaking change?
The only minor change that might be breaking is regarding the ItemKey. If the PrimaryKey is provided and not the ItemKey for an object, the PrimaryKey will be set as the ItemKey. Line number is 175.