Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Conversation

@Satoshi-Sh
Copy link
Contributor

@Satoshi-Sh Satoshi-Sh commented Nov 5, 2023

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)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

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

@vercel
Copy link

vercel bot commented Nov 5, 2023

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

Copy link
Member

@kkrishguptaa kkrishguptaa left a comment

Choose a reason for hiding this comment

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

I fail to understand the need of a heading component with the isAuth prop; I think we could use default heading styles, but I don't understand why do we need a heading component which is different for auth

@Satoshi-Sh
Copy link
Contributor Author

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

  • Keep the isAuth since styles are different
  • Remove isAuth and give all the classes including classes with default value
  • Don't use H2 component for h2 in auth pages.

What solution do you think is better?

@xkrishguptaa

@amandamartin-dev
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

amandamartin-dev commented Nov 6, 2023

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
Copy link
Contributor Author

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

@eddiejaoude
Copy link
Member

Great collaboration everyone 💪 my thoughts...

  • I prefer keeping the same style for h2 everywhere, including authenticated section - so when someone uses h2 they don't need to think about the styling, plus we can update it if required in 1 place
  • I forgot about base styles (thanks for making me aware Amanda), but I personal prefer to keep it in the component, it is easier to tweak/update and see the changes

@Satoshi-Sh
Copy link
Contributor Author

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

@eddiejaoude
Copy link
Member

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
Copy link
Contributor Author

I tried to incorporate what Eddie said here.

Here is what I do.

  • 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?)
  • Added the default style from the main h1 to the H1 component.

@eddiejaoude
Copy link
Member

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 💯

Copy link
Member

@kkrishguptaa kkrishguptaa left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you

@kkrishguptaa kkrishguptaa merged commit e7dea93 into EddieHubCommunity:main Nov 13, 2023
@kkrishguptaa kkrishguptaa added the enhancement New feature or request label Nov 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Move all h1, h2... to reusable react components

4 participants