Skip to content

feat(tables): update tables, context selector#3474

Merged
evwilkin merged 2 commits into
patternfly:v5from
nicolethoen:rewrite_props_tables
Apr 11, 2023
Merged

feat(tables): update tables, context selector#3474
evwilkin merged 2 commits into
patternfly:v5from
nicolethoen:rewrite_props_tables

Conversation

@nicolethoen

@nicolethoen nicolethoen commented Apr 7, 2023

Copy link
Copy Markdown
Collaborator

Closes: #3469

@patternfly-build

patternfly-build commented Apr 7, 2023

Copy link
Copy Markdown
Collaborator

@thatblindgeye thatblindgeye left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In addition to the file comment below:

  • Could we update line ~81 of the sideNavLayout.js file from alignment=... to the new prop align=...? Right now in the surge the group of items that has the version switcher isn't aligned correctly
  • Should we open a followup issue to add back in the sorting functionality for the CSS table? Not really a blocker to include it in this PR, and also depends if we want to add the sorting back in

@nicolethoen

Copy link
Copy Markdown
Collaborator Author
  • Should we open a followup issue to add back in the sorting functionality for the CSS table? Not really a blocker to include it in this PR, and also depends if we want to add the sorting back in

I could try to add it back in. when I was playing with it, it didn't feel like the sorting was adding very much. But i think I have a bit more time to take a second stab at it.

@tlabaj

tlabaj commented Apr 10, 2023

Copy link
Copy Markdown
Contributor
  • Should we open a followup issue to add back in the sorting functionality for the CSS table? Not really a blocker to include it in this PR, and also depends if we want to add the sorting back in

I could try to add it back in. when I was playing with it, it didn't feel like the sorting was adding very much. But i think I have a bit more time to take a second stab at it.

@thatblindgeye @nicolethoen Is it possible to tackle as a follow up since we need this to land to unblock development in React?

@evwilkin evwilkin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

return icon.Name.toLowerCase().includes(searchValue.toLowerCase()) ||
icon.Contextual_usage?.toLowerCase().includes(searchValue.toLowerCase()) ||
icon.Extra_context?.toLowerCase().includes(searchValue.toLowerCase()) ||
icon.Label?.toLowerCase().includes(searchValue.toLowerCase()) ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggest including React_name in the search

Suggested change
icon.Label?.toLowerCase().includes(searchValue.toLowerCase()) ||
icon.Label?.toLowerCase().includes(searchValue.toLowerCase()) ||
icon.React_name.toLowerCase().includes(searchValue.toLowerCase()) ||

</Td>
<Td data-label="Contextual usage">
{recommendation.map(rec => {
return rec.iconUsage

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should only display iconUsage for recommended icons (aka 'new' icons), and add block spacing to avoid results with multiple descriptions from running together (see pficon-dragdrop row in PR preview for 3 contextual usages running together)

Suggested change
return rec.iconUsage
if (rec.iconType === 'new') {
return (
<div key={`${rec.style}-${rec.iconName}-contextual-usage`}>
{rec.iconUsage}
</div>
)
}

Comment on lines +72 to +83
const filteredRows = React.useMemo(() => {
return recommendationsArray.filter(recommendation => {
const oldRec = recommendation[0];
const newRec = recommendation[1];
return oldRec.iconName.toLowerCase().includes(searchValue.toLowerCase()) ||
oldRec.iconUsage.toLowerCase().includes(searchValue.toLowerCase()) ||
oldRec.type.toLowerCase().includes(searchValue.toLowerCase()) ||
newRec.iconName.toLowerCase().includes(searchValue.toLowerCase()) ||
newRec.iconUsage.toLowerCase().includes(searchValue.toLowerCase()) ||
newRec.type.toLowerCase().includes(searchValue.toLowerCase())
});
}
}, [searchValue]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is hard coded to change 2 results (oldRec & newRec), but some rows have up to 5 results (4 old icons, 1 new icon) so suggest refactoring for a more dynamic approach to cover all use cases. Also incorporates a separate recursive function to pull text out from children for the case of Spinner which otherwise was breaking upon search due to passing nested JSX rather than a searchable string to the iconUsage field:

const concatChildText = (children, text = '') => {
  React.Children.forEach(children, (child) => {
    if (typeof child === 'string') {
      text += child;
    } else if (React.isValidElement(child)) {
      text = concatChildText(child.props.children, text);
    }
  });
  return text;
}
const filteredRows = React.useMemo(() => {
    return recommendationsArray.filter(recommendation => (
      recommendation.some(rec => {
        return Object.values(rec).some(val => {
          const cellText = concatChildText(val);
          return cellText.toLowerCase().includes(searchValue.toLowerCase());
        });
      })
    ))
  }, [searchValue]);

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.

update props/css vars tables to composable table

5 participants