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

CSS enchancements #305

Open
ciaranschutte opened this issue Aug 30, 2023 · 0 comments
Open

CSS enchancements #305

ciaranschutte opened this issue Aug 30, 2023 · 0 comments
Assignees
Labels
new-feature Request is a new feature

Comments

@ciaranschutte
Copy link
Contributor

Detailed Description

Some repetition on styling and incorrect markup.
Important to change due to "feature creep" & correct semantic tags are needed for accessibility and correct crawling

P2 "doesn't make sense"
https://github.com/overture-stack/website/blob/develop/src/components/Typography/index.js#L32
The reason is <h1>,<h2>,<h3> and so forth are actual html elements.
They have semantic meaning attached to them.

example: the main page title might be h1 and then a list of blog posts on the page might have h3 heading.
paragraph , <p> is just a paragraph. there's no <p2> because it has a single level of meaning

https://github.com/overture-stack/website/blob/develop/src/components/Typography/styles.scss#L76
The style for p2 only change for breakpoints - all other attributes are the same. This is a sign that there's likely a change could be made to have one element, one style - often this is a very very slight design variable isn't in line with the rest of the design. As seems to be the case here, line-height is the only different attribute.

Once code starts branching into x1, x2, x3 etc for minor changes and the codebase gets established, things become messy, and then no one wants to fix it - good to nip it in the bud.

possible impl

Firstly double check that this design is correct.
Ensure line-height has to change in this situation.
If yes, I would make an extra class to apply just the line-height style, because it's likely to occur again in the design

Then you can have a single

element and a single style.

detail

  1. minor naming.
    https://github.com/overture-stack/website/blob/develop/src/components/Typography/index.js#L32
    Please rename this component to
      -so it matches with the correct web you're using
      similar to <p> , L1 isn't a thing, only <li> and in this case the element you're wrapping is <ul> not <li>
      So reading <Li><li>item</li></Li> is very confusing

feel free to slack me for more info/guidance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature Request is a new feature
Projects
None yet
Development

No branches or pull requests

2 participants