Skip to content

Conversation

@nicolethoen
Copy link
Contributor

@nicolethoen nicolethoen commented Mar 17, 2023

What: Closes #8073

hopefully to aid in review, the components, examples, and demos updated are linked below:

More updated examples after rebasing:
Table examples/demos:

  • sortable with custom control
  • deprecated sortable with custom control
  • sortable responsive demo
  • column management demo - replaced nonfunctional optionsmenu with a menutoggle

A few additional notes:

  • I renamed the Select demos in the demo-app to Select Deprecated for clarity and updated the demos to import select
  • There are a number of places where the existing deprecated select is representing a filter, but it was only partially implemented, or does not expand/collapse at all. In each case I replicated the existing behavior. If we want to update any of these demos to make them more fully functional, or more basic w/o functional filters, we can open a follow up issue.

@patternfly-build
Copy link
Collaborator

patternfly-build commented Mar 17, 2023

@nicolethoen nicolethoen marked this pull request as draft March 17, 2023 15:25
@nicolethoen nicolethoen force-pushed the deprecate_select branch 2 times, most recently from 21620bc to 15747c6 Compare March 20, 2023 14:22
@nicolethoen nicolethoen marked this pull request as ready for review March 20, 2023 14:26
@nicolethoen nicolethoen requested a review from kmcfaul March 20, 2023 14:26
@nicolethoen nicolethoen requested review from gitdallas and tlabaj March 22, 2023 12:45
Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

Everything looks good to me once the CI issues are resolved.

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Same as ☝🏼 , only have one comment below that isn't a blocker if we're able to get it in today

@tlabaj tlabaj requested a review from mcarrano March 27, 2023 13:51
Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@nicolethoen @tlabaj The problem I have with this is that if I look at this from a consumer's perspective, it looks like we've taken a step back. The new default Select implementation has no documentation and on the surface looks to have very limited capability. I understand that the objective was to not reproduce all existing examples here and encourage use of the composable approach, but how would I know that here? I'm willing to help document and define additional examples that we should expose if you agree.

@tlabaj
Copy link
Contributor

tlabaj commented Mar 27, 2023

@nicolethoen @tlabaj The problem I have with this is that if I look at this from a consumer's perspective, it looks like we've taken a step back. The new default Select implementation has no documentation and on the surface looks to have very limited capability. I understand that the objective was to not reproduce all existing examples here and encourage use of the composable approach, but how would I know that here? I'm willing to help document and define additional examples that we should expose if you agree.

@mcarrano Yes, we can add more documentation here. What examples are we looking to add?

@nicolethoen
Copy link
Contributor Author

could building out more examples/docs be a follow up issue here? @mcarrano @tlabaj

@mcarrano
Copy link
Member

could building out more examples/docs be a follow up issue here? @mcarrano @tlabaj

I'm OK with that if you prefer, @nicolethoen . I can open an issue.

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@nicolethoen I found two issue in my UX review:

  1. For the Card demos with a plain select in the upper left of the card, the down caret is missing.

Screenshot 2023-03-28 at 1 39 02 PM

  1. You notes that in the Filterable Table demo, the tool buttons misalign. That's true. But what I'm also seeing is that the Filter icon is incorrect. It should be like the one below. This seems to be consistently wrong everplace this icon is used. Not sure if that's what's also causing the misalignment, but the icon mismatch is concerning. Any idea what's going on there?

Screenshot 2023-03-28 at 1 45 36 PM

Here's what this looks like in the PR preview:

Screenshot 2023-03-28 at 2 01 45 PM

@nicolethoen
Copy link
Contributor Author

@mcarrano when i look at patternfly.org's icons page, I see that there are two filter icons: https://www.patternfly.org/v4/guidelines/icons. One is the pf-icon filter, which is the one you see in the core and react workspaces's lists of icons. the one you're referring to is the font awesome filter icon, which I don't know how to specifically use rather than the pf icon.

@mcarrano
Copy link
Member

mcarrano commented Apr 3, 2023

@nicolethoen according to what's on the Icons page, we should be replacing using of pf-icon-filter with fa-filter.

Screenshot 2023-04-03 at 9 33 45 AM

This was correct previously when using FilterIcon from React, so what changed? @mcoker any idea?

@nicolethoen
Copy link
Contributor Author

@mcarrano i've opened the following issue to note the discrepancy so we don't hold up this PR

@evwilkin
Copy link
Member

evwilkin commented Apr 4, 2023

@nicolethoen put up a fix for the filter icon here - #8920

It's a small change so if you prefer to add it to this PR and close out mine go for it, otherwise my PR can compliment this one to fix the issue - it's unrelated to your changes here and comes from a prior Core bump PR.

@nicolethoen
Copy link
Contributor Author

@evwilkin I'll leave your changes in your PR

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@tlabaj
Copy link
Contributor

tlabaj commented Apr 10, 2023

Whatever can be going on here? https://patternfly-react-pr-8825.surge.sh/components/card/react-demos/#utilization-card-3

Screenshot 2023-04-10 at 10 34 48 AM

@mcarrano this issue was introduced prior to this PR. We can you open a separate issue to look into it

@mcarrano
Copy link
Member

That sounds good @tlabaj . If you can open that issue, I will go ahead and approve this as it otherwise looks good.

@tlabaj
Copy link
Contributor

tlabaj commented Apr 11, 2023

@mcarrano issue opened #8936

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@tlabaj thanks for opening the issue. In that case, this PR looks good to go.

@tlabaj tlabaj merged commit 6fd4eeb into patternfly:v5 Apr 11, 2023
@patternfly-build
Copy link
Collaborator

Your changes have been released in:

  • @patternfly/react-code-editor@5.0.0-alpha.64
  • @patternfly/react-core@5.0.0-alpha.63
  • @patternfly/react-docs@6.0.0-alpha.69
  • demo-app-ts@5.0.0-alpha.47
  • @patternfly/react-integration@5.0.0-alpha.23
  • @patternfly/react-table@5.0.0-alpha.65

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Select - deprecate select

9 participants