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

✨ Create Dashboard Filter Component (#1983) #2006

Merged
merged 18 commits into from
Jul 30, 2021

Conversation

mistryrn
Copy link
Contributor

@mistryrn mistryrn commented Jul 23, 2021

Description of changes

Adds a new DropdownPanel component to UIKit which consists of:

  • a trigger button (contents can be a UIKit icon, or anything you want by using the customTrigger prop)
  • a fully accessible panel which opens/closes on button click, and additionally closes on click/focus outside the panel
  • panel contents are passed as children to the DropdownPanel component

Storybook: https://feat-1983-donor-id-table-filter--argo-ui-storybook.netlify.app/?path=/story/uikit-dropdownpanel--basic

Also includes a [WIP] instance of the DropdownPanel being used as a filter in the column header for the Dashboard's Donor Summary Table to filter based on the Donor ID column

As well as some smaller changes to ensure the new component works:

  • Add a new "filter" icon
  • Extend InteractiveIcon's hover functionality through props to allow the parent to specify different hover colours
  • Make Tooltip component arrow optional
  • Upgrade the react-tippy tooltip library
  • Wrap Input component in forwardRef to allow ref sharing
  • Add onBlur and type props to Button component to allow for focus change handling and to specify type="submit" vs type="button"
  • ReactTable styling changes to shrink the width of the draggable area for column resizers to prevent collision with filter buttons in column headers

Type of Change

  • Bug
  • Styling
  • New Feature

Checklist before requesting review:

  • Design (select one):
    • Matches design:
      • component sizes, spacing, and styles
      • font size, weight, colour
      • spelling has been double checked
    • No design provided, screenshot of changes included in PR
    • No changes to UI
  • UI Kit (select one):
    • New Components with storybook stories created
    • Modified Existing components and updates stories
    • No new UIKit components or changes
  • Feature is minimally responsive
  • Manual testing of UI feature
  • Add copyrights to new files
  • Connected ticket to PR

@mistryrn mistryrn marked this pull request as ready for review July 29, 2021 14:04
@@ -43,6 +43,21 @@ export const StyledTable = styled<typeof ReactTable, StyledTableProps>(ReactTabl

&.ReactTable {
border: none;
&.has-filters {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick explainer on these table styles:

  • for tables with header filters, we want to prevent the opacity change when there is no data so that the filters still look interactive
  • for tables with header filters, we want to make sure the table has a min-height so that the filter panel can be visible when open, regardless of the amount of data in the table
  • because of the min-height, we need to apply the negative margin-bottom when there is no data so that the "No data found in table" component appears as normal on tables with filters

Kind of hacky, but this classname solution seemed like the least intrusive approach to make sure everything still works as expected

@@ -221,10 +233,10 @@ export default css`
&.ReactTable .rt-resizer {
display: inline-block;
position: absolute;
width: 36px;
width: 12px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick explainer for these rt-resizer style changes:

  • because we're adding filter buttons inside the table headers, we need to shrink the touch area for the column resizers so they don't over lap the new filter buttons

@justincorrigible
Copy link
Member

Updated Storybook with the latest changes

* Adds a new "Filter" icon to the UIKit icon collection
* Adds support for custom fill colour on hover of interactive icons
* Upgrade react-tippy from v1.2.3 to v1.4.0
* Use TooltipProps from react-tippy library instead of exposing them manually
* Make tooltip arrow optional
* Adds boolean prop `arrow` to Tooltip for controlling the visibility of the tooltip arrow
    * Upgrade react-tippy from v1.2.3 to v1.4.0
    * Use TooltipProps from react-tippy library instead of exposing them manually
* Allows Button component to accept `onBlur` handlers as props
* Enables ref forwarding to Input component instances
* Enables specification of button types `'button' | 'submit' | 'reset'`
* Adds a new `DropdownPanel` component to UIKit which renders a trigger button that, upon clicking, expands a panel which can be populated by content passed in as `children`
* Implements the Donor ID Filter to the Donor Summary Table via a custom column header containing a DropdownPanel with a text filter
* TODO: connect the filter text to the API
* Moves code related to the text input filter for dropdown panels out of the Donor Summary Table page and into the DropdownPanel component. Cleans up imports, keeps everything related to the panel in one location for better reuse later on.
* Fixes a11y issue where clear button on UIKit Input component was inaccessible to non-mouse users
* Update Program Donor Summary query to support filters
* Connect UI to Donor ID filter API
* Set table header filter icon to blue when filter is active, regardless of whether or not the panel is open, to indicate that it is currently being applied
* Apply `min-height` to table so the panel can be used without scrolling when there isn't enough data
* Prevent table header opacity change when all data is filtered out
@justincorrigible
Copy link
Member

justincorrigible commented Jul 30, 2021

Rebasing PR over develop to test if build pipeline takes it


edit: it's alive!

sorts,
filters,
options = {},
}: {
Copy link
Member

Choose a reason for hiding this comment

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

💪

// exposing full react-tippy API based on https://github.com/tvkhoa/react-tippy
// extending the html prop to support our previous implementation which also accepted strings
export type TooltipProps = Omit<TippyProps, 'html'> & {
html?: React.ReactElement<any> | React.ReactNode | string;
Copy link
Member

Choose a reason for hiding this comment

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

man, that's clever!!! I saw this as a blocker with UiKit, because they hadn't updated their type stuff

@@ -53,7 +53,7 @@ const TooltipStories = storiesOf(`${__dirname}`, module).add(
transitionFlip: boolean('transitionFlip', undefined),
unmountHTMLWhenHide: boolean('unmountHTMLWhenHide', undefined),
sticky: boolean('sticky', undefined),
stickyDuration: number('stickyDuration', undefined),
stickyDuration: boolean('stickyDuration', undefined),
Copy link
Member

@justincorrigible justincorrigible Jul 30, 2021

Choose a reason for hiding this comment

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

This is the type issue I found at tippy! grr

There's been a PR to fix it since 2020
tvkhoa/react-tippy#133

<StyledInputWrapper
size={size as StyledInputWrapperProps['size']}
onClick={(e) => {
if (inputRef.current && e.target !== clearButtonRef?.current) inputRef.current.focus();
Copy link
Member

@justincorrigible justincorrigible Jul 30, 2021

Choose a reason for hiding this comment

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

yeah, events' prevent default and bubbling control was iffy in React 16.

I can see the hoops you've had to jump through to make this one work... 💪

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely pulled some hair out and cursed at TypeScript quite a bit during this :P

Copy link
Member

Choose a reason for hiding this comment

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

Nah, it's React. They rewrote all that stuff in v17.

Copy link
Member

@justincorrigible justincorrigible left a comment

Choose a reason for hiding this comment

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

Made a couple (non-show-stopping) suggestions, which you should feel free to add or dismiss...
This PR looks pretty darn good otherwise. Ready to squash and merge! 💪

Of note, I've committed a fix for the Input's declaration, so Storybook can identify it correctly again.
(Updated the temporary storybook link, for verification)

@mistryrn mistryrn merged commit ccc7edb into develop Jul 30, 2021
@justincorrigible justincorrigible deleted the feat/1983-donor-id-table-filter branch July 30, 2021 18:38
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.

2 participants