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

Add sistent-divider-component #6048

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

mdkaifansari04
Copy link
Contributor

Description
Added Sistent Divider component with detailed documentation and clean examples
This PR fixes #5917

Notes for Reviewers
Docs Screenshots
image
image
image

Signed commits

  • Yes, I signed my commits.

@github-actions github-actions bot added area/projects An issue relating to Layer5 initiatives (projects) project/sistent labels Nov 1, 2024
@l5io
Copy link
Contributor

l5io commented Nov 1, 2024

🚀 Preview for commit b43e1fe at: https://672545b8e00adfb0dac1599f--layer5.netlify.app

@nebula-aac nebula-aac self-requested a review November 2, 2024 20:51
@vishalvivekm
Copy link
Contributor

@mdkaifansari04
Let's discuss this on websites call on Monday at 6:30 PM IST (7:00 AM CT). Add it as an agenda item to the meeting minutes, if you would :)

@mdkaifansari04
Copy link
Contributor Author

I'll be happy to discuss this on meeting

@mdkaifansari04 mdkaifansari04 force-pushed the feat/sistent-divider-component branch from cfcb2bc to c1efa8c Compare November 5, 2024 21:20
@l5io
Copy link
Contributor

l5io commented Nov 5, 2024

🚀 Preview for commit c1efa8c at: https://672a90c2dd3a3756f3b53764--layer5.netlify.app

@Vidit-Kushwaha
Copy link
Contributor

Hello, @mdkaifansari04, can you remove the excess indentation from the array of code in the code example so that it looks more uniform?

@mdkaifansari04
Copy link
Contributor Author

Sure I'll consider fixing that @Vidit-Kushwaha

@mdkaifansari04
Copy link
Contributor Author

@Vidit-Kushwaha Can you please check it, if its perfect now

@l5io
Copy link
Contributor

l5io commented Nov 6, 2024

🚀 Preview for commit 82201de at: https://672bc9cf447ceb3ac104eed3--layer5.netlify.app

@NishantSinghhhhh
Copy link
Contributor

You PR looks fine , but it will look more good if you add a section in which you will explain about the Props that will be used while working with this component.

@mdkaifansari04
Copy link
Contributor Author

@NishantSinghhhhh It would be very helpful if you could explain this a bit

@NishantSinghhhhh
Copy link
Contributor

Screenshot_20241111-193310.png

Can you add something similar to this ?

@mdkaifansari04
Copy link
Contributor Author

Thanks @NishantSinghhhhh, it was very helpful though

@l5io
Copy link
Contributor

l5io commented Nov 11, 2024

🚀 Preview for commit 1b49a8f at: https://673245855c8bad752448ff53--layer5.netlify.app

Signed-off-by: Mdkaif-123 <[email protected]>
Signed-off-by: Mdkaif-123 <[email protected]>
@l5io
Copy link
Contributor

l5io commented Nov 11, 2024

🚀 Preview for commit 089e64c at: https://67325511f7863492bcf6869c--layer5.netlify.app

<Divider variant="inset" component="li" />
<ListItem>
<ListItemText primary="Middle variant below" />
</ListItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot_20241112-093749.png

Can you please align the lorem ipsum lines with the other text?

}

h5 {
padding: 2rem 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please tell me why are you changing styles of this section?

Copy link
Contributor

@Vidit-Kushwaha Vidit-Kushwaha left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Copy link

@ianrwhitney ianrwhitney left a comment

Choose a reason for hiding this comment

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

Overall looks great. Made a few suggestions for your consideration. Thank you.

</List>
</SistentThemeProvider>
</div>
<CodeBlock name="variant-example" code={codes[0]} />

Choose a reason for hiding this comment

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

Curious if it would make this more readable if instead of using array position for codes we use some map w/ a meaningful id so that if someone has to change it they will get some context of what each element in the array is.

"& > :not(style) ~ :not(style)": {
marginTop: theme.spacing(2),
},
}));

Choose a reason for hiding this comment

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

anyway we can simplify this?

<a id="Different Ways to Use Divider">
<h2>Ways to use Divider Component</h2>
</a>
<p>Here are ways to use the Divider component effectively:</p>

Choose a reason for hiding this comment

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

[nit] Here are a few ways ...

<br />
<p>
If you're using the Divider to wrap other elements, such as text or
chips, we recommend changing its rendered element to a plain

Choose a reason for hiding this comment

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

curious if a new contributor reading this doc will know what chips are? Idk if a link or referencing it as a component will make this more clear?

<h3>Accessibility</h3>
</a>
<p>
Due to its implicit role of separator, the Divider, which is a

Choose a reason for hiding this comment

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

[nit] "Because the Divider serves as an implicit separator, it is announced by screen readers as a..."

@mdkaifansari04
Copy link
Contributor Author

mdkaifansari04 commented Nov 20, 2024

Thanks @ianrwhitney for your feedback, I'll make sure update it accordingly

@vishalvivekm vishalvivekm requested a review from SAHU-01 December 2, 2024 13:40
@Vidit-Kushwaha
Copy link
Contributor

Hi @mdkaifansari04 Facing issues merging your PR in Sistent?
We are conducting a special tutorial on this Monday, December 16, 2024, at 7 AM Central (6:30 PM IST) in the website meeting.

We'll cover:

  • Navigating the Sistent repo
  • Setting up locally
  • Helping your initial pull request to be merged

Get more information: here

👥 Get involved:

Don’t miss out. 🚀

@sudhanshutech
Copy link
Member

@mdkaifansari04 are you completing it?

@mdkaifansari04
Copy link
Contributor Author

Yes @sudhanshutech I have it locally but currently involved in end sem exam.

I'll surely make the changes as soon as possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/projects An issue relating to Layer5 initiatives (projects) project/kanvas project/sistent
Development

Successfully merging this pull request may close these issues.

[Sistent] Add Divider component to the sistent components page
8 participants