-
Notifications
You must be signed in to change notification settings - Fork 215
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
Component: VHeading and VText #546
Comments
Is it possible to block this issue for a few days? This since the unit-less approach shared in WordPress/openverse-frontend#1403 and my current review of text styles would need a second round to match the line-height outcome in Figma and code. I would need some help actually testing the text styles in code. Codepen sounds enough for this task. |
@panchovm yes of course 🙂 Take your time. Codepen does sound like a good place to test them out. Let me know if you need any help 🚀 |
Coming back to this ticket asking for reviewing this need based on the current work done with #415. Since you @sarayourfriend created the ticket, can you revisit whether we still need this change or not? Otherwise, please ping whoever you consider necessary to jump in. |
I still feel like this could be useful. While we do have classes like |
+1 to Dhruv's reasoning. |
I'm removing this from the Core UI improvements project because this should be a change which does not actually change the user interface, just the underlying implementation. @fcoveram can you confirm this? Were there any plans to change heading sizes or styles? I don't believe so but want to make sure. |
No. No changes so far. I thought this suggestion was going to be addressed by @obulat. If this ticket belongs to a different stage of #415 and its scope, then removing it from the project sounds better. Thanks for jumping in @zackkrida |
Description
To ensure consistency and easy of updating typography across the app, it would be nice to have a VHeading and a VText component to encapsulate and apply styles to copy.
I think it's fine to implement these in the same PR. Just make sure to create a Storybook page that is descriptive and shows all the styles with a snapshot test.
Alternatively, rather than creating components, we could also add new classes to our
tailwind.css
in the@base
layer and just use those to style the copy. I personally dislike this option because it doesn't lean on the benefits that a component would have (like a quick way to find all the specific variants of text we support without having to wade through the CSS implementing them, i.e., the prop definition, plus the TypeScript support to check if the variant passed actually exists!). Either is probably fine though 🙂 We just need some way of applying styles to copy consistently throughout the application that can also be updated without massive code uplift.Additionally, with the component approach we could unify the approaches and just have a single VText component. VHeading could alternatively be implemented over VText to pass specific
variant
andas
values for any givenlevel
:I'm skeptical whether it should be possible to render something that visually appears to be a heading but isn't actually a heading though (which adding the heading styles to VText's variants would allow for).
API
Props
For both VHeading and VText, follow the styles laid out in this Figma node to determine what the
variant
andlevel
props should render, style wise: https://www.figma.com/file/GIIQ4sDbaToCfFQyKMvzr8/Openverse-Design-Library?node-id=311%3A331cc @panchovm can you confirm that those are correct, at least for today's designs?
Slots
Events
Bind all listeners to the root element.
References
Implementation
The text was updated successfully, but these errors were encountered: