-
Notifications
You must be signed in to change notification settings - Fork 365
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
feat: [M3-6754] - [on hold] Create new react list component #9594
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per my comments, the API is a little too open IMO, and makes an "everything is possible" component, which I'd like to flag. I wonder about a component that accepts "deletableItems" & "selectableItems" props and contain that logic as well.
Also, why are we not using the multiSelect component (or a variation of it at least) which achieves all this functionality? (ex: Linode select - https://cloud.linode.com/firewalls/create)
20, | ||
].map((num) => { | ||
return { id: num, label: `my-linode-${num}` }; | ||
}) as Linode[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Write either a loop or
const listItems = Array.from({ length: 20 }, (_, index) => {
const num = index + 1;
return { id: num, label: `my-linode-${num}` };
});
<StyledLink | ||
style={{ textDecoration: 'none' }} | ||
to={`/linodes/${linode.id}`} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you have a wrapper with a "button" role that contains a link to the entity. <ListItemButton />
could be removed all together and its styles moved to the StyledLink
element
You can see that this element adds a hover state and the ripple when you click, but then clicking on it does nothing unless you are on the link itself
lastly, for consistency, this type of styling should also be on the core component, not on its consumer. I feel like such list should look the same whatever item is passed to it. For accessibility and markup semanticity, the ScrollableList
should be a <ul>
and we should require it's children to be <li>
. may feel restrictive but it's the right thing to do as an initial component
@abailly-akamai Thanks for the feedback, and good point about the multiSelect component -- I'll ask UX about using this rather than having a new list-like component, especially if the multiSelect already has all the functionality needed. Will work on the other feedback soon too! |
@abailly-akamai spoke with UX, one of the main reasons to not use the multi-select component was due to how the selected linodes appeared at the top -- if there are too many linodes selected, it could get cluttered. Depending on a linode label's length, multiple linodes might appear on the same line of the box at the top: Might be worth looking into this component further to see if it's possible for each linode label tag to be on a separate line |
@coliu-akamai thanks for checking with UX - I think there's a disconnect between UX requirements and what we're trying to achieve here. We shouldn't have to build a whole new series of components an achieve existing functionality with a UI variation - will continue this discussion with UX outside of this PR |
Closing based on 8/31 Frontend Cafe discussion as we won't be moving forward with this approach -- we will either go with Jaalah's autocomplete (which allows for multiselect) or the existing table component (similar to what's done in the Server Transfers page). |
turns out I was working off the wrong designs 😢
brb + ON HOLD until decision meeting about new component vs using existing multi select
Description 📝
Major Changes 🔄
Preview 📷
(this would be used for the unassign linodes drawer)
(this would be used for the assign linodes drawer)
How to test 🧪
yarn storybook
--> ScrollableList