-
Notifications
You must be signed in to change notification settings - Fork 573
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(Ladder): new component #976
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
d0764b5
to
5e0e209
Compare
@DarkGhostHunter Thanks for this PR! Don't you think it would be better to call this component |
Also, would it make sense to support links? |
Yep, that's okay. I thought of a
Well, it does with Its tricky to support links because the link would:
So I went with the easy route only supporting the click on the icon. Now that you mention it, I should add a hover effect. As previous attempts show, creating this type of element is tricky and doesn't come without its own limitations and issues, primarily because the design of the step and the separators themselves. Some concessions on functionality and design must be made to avoid unwanted or shallow behavior. |
I've encountered this issue on the |
Also, it would be nice to move this component to the |
type: Array as PropType<String[] | LadderStep[]>, | ||
default: () => [] | ||
}, | ||
separators: { |
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.
Could be nice to call this divider
for consistency.
type: Number, | ||
default: 0 | ||
}, | ||
steps: { |
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.
Maybe we can call this items
like in Feed
?
return [...appConfig.ui.colors, 'gray'].includes(value) | ||
} | ||
}, | ||
click: { |
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.
Shouldn't this be handled in items
directly and emit a click
instead of using a prop?
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.
You know? You're right if you think that the normal implementation for this would be to not have any click behavior and let the changes be done outside, like a form.
I just thought that it would be faster to implement a way to let the dev handle the click for changing steps in one variable rather than adding the handler on each item.
Can I set then the change-on-click
prop to add a default click handler to all items to change to that step when clicked?
Yeah, but here the separstor would be tricky to put unless... ...we add four separators. Two each side of the icon, and two each side of the whole step. This way the looks like the separators are one. To align them, we should use a 3x2 CSS grid instead, that way the separators on the top can be naturally aligned in the center of the first row, where the icon is.
Go ahead. |
@DarkGhostHunter Do you plan to keep working this PR or should I take over? |
You may take over. The time I had left to work on it is gone π₯ |
Any news? |
Hey ! Is there any news on this PR ? Thanks :) |
I haven't found the time to finish this unfortunately. I think this will be dropped and implemented in |
Me neither. I have some nitpicks about the implementation, but either way it's better to wait for radix, or push a PR there. |
π Linked issue
Resolves #180
β Type of change
π Description
This is my interpretation of #549. While is not that simplier like the aforementioned, it's more flexible in design:
While I tried to make it vertically compatible, it's impossible without making spaghetti code to support both orientations. Instead, we should grab inspiration from Tailwind UI Feeds.
π Checklist