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

docs(Table Filter Demo) #3127

Merged
merged 8 commits into from Oct 25, 2019
Merged

docs(Table Filter Demo) #3127

merged 8 commits into from Oct 25, 2019

Conversation

@kmcfaul
Copy link
Contributor

kmcfaul commented Oct 11, 2019

What: Adds a demo of a filterable table. Adds a property to DataToolbarFilter (to allow filter chips to be displayed without the toolbar dropdown being displayed), and fixes a console warning in Select (create&no data text props were being passed to the DOM)

Refer to issue: #923

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Oct 11, 2019

PatternFly-React preview: https://patternfly-react-pr-3127.surge.sh

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 11, 2019

Codecov Report

Merging #3127 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3127      +/-   ##
==========================================
- Coverage   69.03%   69.03%   -0.01%     
==========================================
  Files         859      859              
  Lines       23637    23641       +4     
  Branches     1895     1895              
==========================================
+ Hits        16318    16320       +2     
- Misses       6359     6361       +2     
  Partials      960      960
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.3% <ø> (ø) ⬆️
#patternfly4 68.05% <100%> (-0.01%) ⬇️
Impacted Files Coverage Δ
...nfly-4/react-core/src/components/Select/Select.tsx 62.17% <100%> (+0.39%) ⬆️
...y-react-extensions/src/components/Select/Select.js 19.61% <0%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e77c613...cc4834b. Read the comment docs.

@kmcfaul kmcfaul force-pushed the kmcfaul:table-attr-filter-demo branch from d1e89cf to f518e17 Oct 11, 2019
@tlabaj tlabaj added the Do Not Merge label Oct 11, 2019
@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 11, 2019

Added do not merge to wait for other PRs to go in first

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Oct 14, 2019

@kmcfaul I took a quick peek at this realizing that it still may be WIP. I think it's looking really good. Here are just a few small things I noticed:

All other behavior is working exactly as I expected. Great work!

@tlabaj tlabaj requested a review from nicolethoen Oct 22, 2019
@tlabaj tlabaj added the ux review label Oct 22, 2019
@tlabaj tlabaj assigned mcarrano and unassigned mcarrano Oct 22, 2019
@tlabaj tlabaj requested a review from mcarrano Oct 22, 2019
@kmcfaul kmcfaul force-pushed the kmcfaul:table-attr-filter-demo branch from f518e17 to 679ed13 Oct 22, 2019
@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 22, 2019

@mcarrano Added updates based on your feedback, except the empty state. I cannot get to the full page demo via the link - it returns a 404 page. Is there another place I can look for this? I don't think we have an empty state demo for the table in react yet.

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Oct 22, 2019

Looks like there is a website issue preventing you from seeing the empty state demo @kmcfaul .

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Oct 22, 2019

Actually, here is a screen shot of what the empty state should look like @kmcfaul:

Screen Shot 2019-10-22 at 4 57 05 PM

You should be able to access this under core demos/table Look for the Table-empty state demo

@nicolethoen

This comment has been minimized.

Copy link
Contributor

nicolethoen commented Oct 23, 2019

Is this demo supposed to be in an experimental category?

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 23, 2019

@mcarrano Added the empty state, thanks for the screenshot & location!

@nicolethoen @rachael-phillips We don't have a demos section in Experimental, but maybe it should be added?

@nicolethoen

This comment has been minimized.

Copy link
Contributor

nicolethoen commented Oct 23, 2019

I think we probably don't want to encourage people to implement the toolbar as seen in the demo if regular breaking changes could still be in the component's future. So the demo should be specifically categorized or consumers sufficiently warned... I thought that it was agreed that the demo would be in an experimental section or held off until the component is no longer experimental

When the other PR gets merged, you'll want to add a 'collapseListedFiltersBreakpoint' to the DataToolbar component to make it more responsive, just a heads up. @mcarrano should this filter be in a toggle group so it can be more responsive? adding this prop will make the 'X filters applied' message appear, but without the toggle group, there wont be a way to see which filters have been applied. I'm honestly not sure what the behavior of this multiple drop down select will be when it is used in a toggle group - it should be explored.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 23, 2019

Yeah I agree, I'll try creating a new folder for demos in experimental and see if that gets picked up!

Thanks for the heads up about the PR changes as well.

Copy link
Member

mcarrano left a comment

@kmcfaul The functionality here is looking great. All filter behaviors are as I expect. Thanks for implementing the empty state. The only problem I see is with how this responds as I shrink the viewport. Looks like @nicolethoen pointed this out. also. The filters should be enclosed in a toggle group to get the responsive behavior that we want.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 23, 2019

@mcarrano @nicolethoen
Adding in the toggle creates some issues.

  • The biggest issue is that the chip groups aren't located properly in the collapsed state's expanded menu for this use case. They are located after their respective dropdown, and contain spacing even when no chips are present. With 2 of the 3 dropdowns hidden, there are varying white space gaps above and below each filter dropdown. I would expect all chip groups to be inline at the end of all dropdowns.
  • The inline styling to prevent the dropdowns from resizing also limit them in the collapsed state menu when its expanded. Should I remove these limits?
  • It always appears collapsed despite setting a breakpoint. Might be user error on my part, I'm fiddling with it still.
@kmcfaul kmcfaul force-pushed the kmcfaul:table-attr-filter-demo branch from daeb674 to f479db8 Oct 23, 2019
@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 23, 2019

Talked with Nicole and rebased with the data toolbar changes. The responsive collapse and expansion is working now.

I removed the width styling for now, and scheduled a meeting to go over the chip group locations in the collapsed menu.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 24, 2019

As this demo uses an experimental component, we are moving forward with this PR and will increment on it in the next release re: the chip group stacking in the collapsed menu.

@nicolethoen

This comment has been minimized.

Copy link
Contributor

nicolethoen commented Oct 24, 2019

And people are okay with an experimental component under Demos?

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Oct 24, 2019

I believe that was what was decided after a recent standup. @tlabaj Can you confirm?

@christiemolloy

This comment has been minimized.

Copy link
Member

christiemolloy commented Oct 24, 2019

I wonder if this demo should be under Toolbar demo because in Core it is, and the reason for the demo was to show how the new toolbar works when filtering the table. If it sits under Table demo, I don't know what functionality of the table its demonstrating? Seems like its just demonstrating the filtering. Would be great to hear what @mcarrano thinks

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Oct 24, 2019

This is tricky @christiemolloy . I discussed this with @kmcfaul and @nicolethoen in a meeting earlier today. Naming of demos is something that needs more thought in general, IMO. Should they be based on the thing being demonstrated (i.e. Toolbar) or the use case being demonstrated (i.e. Filtering). I don't know, but the naming will effect how easy it is for developers to find these. Not sure how the website search indexes these (or does it). Let's leave alone for now, but @rachael-phillips @LHinson I think this is a topic that should be on the radar for more discussion.

@tlabaj tlabaj added PF4 and removed Do Not Merge labels Oct 24, 2019
Copy link
Contributor

evwilkin left a comment

LGTM

@mcarrano mcarrano added ux approved and removed ux review labels Oct 25, 2019
Copy link
Member

mcarrano left a comment

Looks good. Thanks for making those changes @kmcfaul

@tlabaj
tlabaj approved these changes Oct 25, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 725af73 into patternfly:master Oct 25, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.