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(clayui.com): Add documentation for examples in form of list of links to their stories #3278
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d67a6ab:
|
Yeah, I think screenshots are probably not a good option, maybe small illustrations of simpler lines, nothing too complex.
Yes I also think it would be a good option, to explore the components. Not everyone knows the components by name, it would be good both to fill space and to facilitate the search for the components. |
@matuzalemsteles I agree with both of your points, do we need illustrations for components too, in that case? Some can get pretty complex very quickly, like Dual List Box for example |
@kresimir-coko yeah, the good thing about the illustrations that don't need to stick to the details of the components, but I'm not sure if that would be a problem. So for more complex components, in the illustrations it will not be necessary to be so faithful, because it will decrease the maintenance cost if the component changes some detail. I think we can go now with the examples and then ask for help from the Design team to create for the components illustrations, due to the current quantity of components. We can also try to do it ourselves. |
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.
Looks pretty good, I am unsure about screenshots for our full page examples though. We could even just list them out as links and I think that would be helpful.
…inks to their stories
Hey @bryceosterhaus, I've made changes to this since monday.
What should be our next step regarding this, should we keep it like this until we get illustrations? |
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.
Just had a comment about using the List component but other than that, looks good.
clayui.com/content/docs/examples.mdx
Outdated
href="https://storybook.clayui.com/?path=/story/demos-templates--list-page" | ||
target="_blank" | ||
> | ||
<ClayList.Item> |
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.
Item
needs to be the first descendant of ClayList
, otherwise it won't be semantic.
Also, I think it'd be better to not use List and instead just ClayLink with line breaks between each item. I think the list looks a little odd for this case.
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.
Makes me wonder whether there should be a check (only when process.env.NODE_ENV === 'development'
) that looks at the children and makes sure there are no non-list children... (or at least no children that are definitely the wrong type of Clay component).
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.
This attempt of mine might have demonstrated a need for a new prop (maybe a boolean link
) for ListItem
. I wanted to make the whole List Item a link, not just the text inside.
What're your thoughts on this prop @bryceosterhaus @matuzalemsteles ?
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.
@kresimir-coko I think we would still have problems with semantics. To reverse this problem we would need to modify ClayList
and ListItem
to not use ul
and li
for cases with Link
or Button
, I don't know if this would be very friendly.
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.
@matuzalemsteles not sure it's not bypassable tho, I think that with some CSS we can make the a
full width and height and thus making the entire area of the li
clickable.
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.
@kresimir-coko I'm not sure what you want to say, would it be changing the CSS to support this? I was referring that we could replace the tag ul
and li
with a div
and a
or button
using the same classes, this is the most recommended for these cases, I suppose Clay already supports this since it extends from the bootstrap.
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.
@matuzalemsteles Yes, I'm suggesting that we keep the ul
> li
> a
hierarchy, and just manipulate the css so that we can have the a
element cover 100% of the li
's size
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.
Oh, now I get it 😁, Hmm 🤔. @pat270 what do you think?
@bryceosterhaus I've removed the ClayList and made the ClayLinks |
Looks good! |
Hey @matuzalemsteles @jbalsas here's a draft for the Examples page.
Here's what I've done:
My thoughts going forward