Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Introduce List component family #516

Merged
merged 22 commits into from
Jul 22, 2020
Merged

Introduce List component family #516

merged 22 commits into from
Jul 22, 2020

Conversation

alexpaxton
Copy link
Contributor

@alexpaxton alexpaxton commented Jul 8, 2020

Why?

Within the currently available components there is no generic "list" component family. The closest components are the DropownMenu and DropdownItems but they are very opinionated and not designed for contexts outside of dropdowns.

In the InfluxDB UI there are a handful of components that resemble a list but each are slightly different. Between those cases, dropdowns, and right click menus there is an opportunity to consolidate and standardize. Why not have one family of components that can serve all these contexts and more?

Changes

  • Introduce List family of components

Notes

  • Instead of creating a separate version of ListItem intended for use with <a> or <Link> there is a prop for that. Due to how the children of ListItem and itself relate having an extra DOM element in between throws off the styling.
  • Compared to the dropdown family the List components are much more composable
  • Favoring backgroundColor and gradient as the means of colorization instead of ComponentColor enum
  • Unlike dropdowns, each component can be colored individually
    • The styles are not dependent on DOM structure anymore
  • Once this PR is merged, List can begin to replace parts of other components such as RightClick, SelectDropdown, and MultiSelectDropdown

Screenshots

Screen Shot 2020-07-15 at 2 19 17 PM
Screen Shot 2020-07-15 at 2 19 28 PM
Screen Shot 2020-07-15 at 2 19 36 PM
Screen Shot 2020-07-15 at 2 19 55 PM
Screen Shot 2020-07-15 at 2 20 13 PM
Screen Shot 2020-07-15 at 2 20 26 PM

Checklist

Check all that apply

  • Updated documentation to reflect changes
  • Added entry to top of Changelog with link to PR (not issue)
  • Tests pass
  • Peer reviewed and approved
  • Signed CLA (if not already signed)

@alexpaxton
Copy link
Contributor Author

alexpaxton commented Jul 8, 2020

Todo:

  • Ensure styles exist for disabled state of items
  • Make onClick optional for static display of information

@alexpaxton
Copy link
Contributor Author

alexpaxton commented Jul 13, 2020

Switched to using InfluxColors / Gradients as the coloration method for list items (instead of ComponentColor).

  • Polish the selected state of indicators with gradients
  • Rename some contexts and add comments in the code
  • Revisit ListLinkItem approach

@alexpaxton alexpaxton requested a review from mavarius July 15, 2020 21:30
Copy link
Collaborator

@mavarius mavarius left a comment

Choose a reason for hiding this comment

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

Just a few notes; looks good though, this will be useful.

*/

.cf-list-item__disabled {
opacity: 0.666;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this opacity is used in several places, would it be worth making it a variable?

export const ListItemContext = createContext<ListItemContextProps>({
selected: false,
size: ComponentSize.Small,
listItemBackgroundColor: undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is undefined necessary here? Seems redundant to define a default prop as "undefined".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The context needs some initial value set. I could do null instead. I could be wrong but I think it will complain if I don't set a value

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ya, defining null would be better than defining undefined.

@mavarius mavarius self-requested a review July 22, 2020 15:58
Copy link
Collaborator

@mavarius mavarius left a comment

Choose a reason for hiding this comment

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

Good to go with just the undefined to null changes.

export type ListItemRef = HTMLDivElement

export const ListContext = createContext<ListContextProps>({
listBackgroundColor: undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can switch these to null too.

@alexpaxton alexpaxton merged commit 9f01a7c into master Jul 22, 2020
@alexpaxton alexpaxton deleted the feat/list-component branch July 22, 2020 16:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants