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(filter-panel): introducing react wrapper #7470

Merged

Conversation

IgnacioBecerra
Copy link
Contributor

Related Ticket(s)

#6644

Description

Introducing the React wrapper for the Filter Panel component.

Changelog

New

  • added comments to the getter functions

Changed

  • included the filter-panel and filter-panel-modal components in filter-panel-composite in order for the wrapper component to properly render

@IgnacioBecerra IgnacioBecerra requested a review from a team as a code owner October 21, 2021 00:30
@IgnacioBecerra IgnacioBecerra added 👀 eyes needed Needs design approval PRs on feature requests and new components have to get design approval before merge. package: web components Work necessary for the IBM.com Library web components package labels Oct 21, 2021
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 21, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 21, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 21, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 21, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 21, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 21, 2021

Deploy preview created for package "Web Components (Codesandbox Examples)":
https://webcomponents-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7470/index.html

Built with commit: 806f9d168609f07b3f3135cb4b318db3fc1956a8

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 21, 2021

Deploy preview created for package "React (Codesandbox Examples)":
https://react-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7470/index.html

Built with commit: 806f9d168609f07b3f3135cb4b318db3fc1956a8

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

I updated the branch, hopefully the changes from #7452 will be reflected in the wrapper afterwards?

@jeffchew
Copy link
Member

You might need to update and do a yarn reset before running snapshot updates again:

image

@oliviaflory oliviaflory self-assigned this Oct 22, 2021
@jeffchew
Copy link
Member

@IgnacioBecerra are there props for the sub components that can be included here?

image

@jeffchew
Copy link
Member

@IgnacioBecerra are there props for the sub components that can be included here?

image

Just checking if you can address this too, I didn't see this updated in the latest revision.

@jeffchew
Copy link
Member

@ariellalgilmore I think I recall seeing this when you first introduced the e2e test for this, wondering if this is something that is happening sporadically based on animation timing?

image

@ariellalgilmore
Copy link
Member

@ariellalgilmore I think I recall seeing this when you first introduced the e2e test for this, wondering if this is something that is happening sporadically based on animation timing?

image

Hmm i added a wait time for the animation to go through after I saw that test failing

@jeffchew
Copy link
Member

@ariellalgilmore I think I recall seeing this when you first introduced the e2e test for this, wondering if this is something that is happening sporadically based on animation timing?
image

Hmm i added a wait time for the animation to go through after I saw that test failing

@ariellalgilmore @annawen1 maybe we should try to disable all animations for tests, something like this?

https://gist.github.com/cvan/576eb41ab5d382660c14e3831c33c6ea

@jeffchew
Copy link
Member

Thanks @IgnacioBecerra ,

I think we should add prop tables for all of the sub components too, and also make it clear which prop table is for which. It looks like you just posted the table for DDSFilterPanelComposite? Should have separate tables for DDSFilterPanelCheckbox, DDSFilterGroupItem, etc.

@IgnacioBecerra
Copy link
Contributor Author

@jeffchew I added tables for InputSelect and InputSelectItem. Some components like FilterGroup and FilterGroupItem don't have props so I excluded those.

I tried adding FilterPanelCheckbox and FilterPanelModal but it seems that the table wasn't rendering for some reason, stating that there are no props were found for those components. Not sure why that's the case, but these components don't require the use of props to work properly as they can just be used as it is (modal is actually embedded in filter panel composite anyway).

@annawen1
Copy link
Member

@ariellalgilmore I think I recall seeing this when you first introduced the e2e test for this, wondering if this is something that is happening sporadically based on animation timing?
image

Hmm i added a wait time for the animation to go through after I saw that test failing

@ariellalgilmore @annawen1 maybe we should try to disable all animations for tests, something like this?

https://gist.github.com/cvan/576eb41ab5d382660c14e3831c33c6ea

hmm we can try it to see if the delay is really coming from the animations

@jeffchew
Copy link
Member

@IgnacioBecerra looks like some formatting issues, can you take a look?

image

@oliviaflory
Copy link
Contributor

@IgnacioBecerra

The animation fix looks good!

I'm going to be a little picky about how the panel is sitting on the grid though, forgive me (please!).

  • The left side looks perfect
  • On the right side, we want it to end on the edge of the column rather than the gutter.
    • Currently both 3 and 4 column variants are ending on the gutter (dotted line)
    • We want it to end on the column (solid line)

3 Column
Screen Shot 2021-10-25 at 4 25 34 PM

4 column
Screen Shot 2021-10-25 at 4 23 44 PM

@IgnacioBecerra
Copy link
Contributor Author

IgnacioBecerra commented Oct 25, 2021

@shixiedesign I changed this in both this new react wrapper story and the original web components story for Filter Panel 👍

edit: meant to ping @oliviaflory! The pixel profile pic threw me off haha

@shixiedesign
Copy link
Contributor

😄 Did you mean to ping @oliviaflory ?

Copy link
Member

@jeffchew jeffchew left a comment

Choose a reason for hiding this comment

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

Thank you @IgnacioBecerra for the updates, LGTM!

Copy link
Contributor

@oliviaflory oliviaflory left a comment

Choose a reason for hiding this comment

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

Looks good @IgnacioBecerra !

@IgnacioBecerra IgnacioBecerra added the Ready to merge Label for the pull requests that are ready to merge label Oct 26, 2021
@kodiakhq kodiakhq bot merged commit b828912 into carbon-design-system:master Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 eyes needed Needs design approval PRs on feature requests and new components have to get design approval before merge. package: web components Work necessary for the IBM.com Library web components package Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants