EddieHubCommunity / EddieHubCommunity.github.io

Information about our community
http://eddiehub.org
Other
153 stars 122 forks source link

feat: componetize h1 h2 #307

Closed Satoshi-Sh closed 11 months ago

Satoshi-Sh commented 11 months ago

Fixes Issue

closes #301

Changes proposed

[x] - Created Heading.jsx for H1 and H2 components [x] - Replace all the h1 and h2 tags with the newly created components

Check List (Check all the applicable boxes)

Screenshots

Note to reviewers

I checked if classes of each element match with the original classes.

vercel[bot] commented 11 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 11, 2023 7:08pm
Satoshi-Sh commented 11 months ago

h2 for auth pages 'mt-20 text-lg font-semibold text-gray-900' h2 for other pages 'font-display text-3xl tracking-tight sm:text-4xl'

I made isAuth since it doesn't have tracking-tight(letter-spacing) and sm:text-4xl. I can overwrite their classes by default values classes.

I think it's better to use H2 for all the h2 for the formality.

Possible Soulutions

What solution do you think is better?

@xkrishguptaa

amandamartin-dev commented 11 months ago

question for discussion - does it really make sense to make a component for something that just needs universal styling? What is the otehr benefit over simply creating styles that affect all h1,h2.... or anyhting else we want styles for in the app that differ from the defaults tailwind offers?

Satoshi-Sh commented 11 months ago

Currently we have the same styles duplicated everywhere, moving these to components would be better

One benefit is to avoid repeating the same classes over and over.

I did the same kind of task before because my project leader wanted to add animation to the heading. We just needed to animate one time inside the component.

I think it's better to ask @eddiejaoude what he thinks.

amandamartin-dev commented 11 months ago

Currently we have the same styles duplicated everywhere, moving these to components would be better

One benefit is to avoid repeating the same classes over and over.

I did the same kind of task before because my project leader wanted to add animation to the heading. We just needed to animate one time inside the component.

I think it's better to ask @eddiejaoude what he thinks.

Apologies for not being more clear. I mean, updating what our headers look like at the base style level in tailwind. Creating header components feels like over-engineering to me when it's just for basic styles like font size.

Applying base style in tailwind

Definitely agree we need one place where changes flow throughout the app, whether a stylesheet update or a component.

Satoshi-Sh commented 11 months ago

Thanks for clarifying, Amanda. Using base style in Tailwind seems a good strategy.

eddiejaoude commented 11 months ago

Great collaboration everyone 💪 my thoughts...

Satoshi-Sh commented 11 months ago

Great. I will check what default style works fine for h1 and h2, and update my pull request.

eddiejaoude commented 11 months ago

Great. I will check what default style works fine for h1 and h2, and update my pull request.

Thank you 👍 I think use the style from the standard pages (because we won't really have any auth pages in this app)

Satoshi-Sh commented 11 months ago

I tried to incorporate what Eddie said here.

Here is what I do.

eddiejaoude commented 11 months ago

Looks good 👍 I would love to get some other maintainers feedback also

Remove isAuth props since this app doesn't use those pages. I just used the h2 tag instead of the H2 component for the auth pages. (Isn't that better to remove (auth) to avoid confusion?)

yes please raise an issue for this, we should remove these unused files 💯