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

Update specificity and className props #49

Closed
rfearing opened this issue Jul 14, 2022 · 1 comment · Fixed by #59
Closed

Update specificity and className props #49

rfearing opened this issue Jul 14, 2022 · 1 comment · Fixed by #59
Assignees
Labels

Comments

@rfearing
Copy link
Contributor

Context:

The compete page on resources hub utilized custom River components. Those components have been switched out for React Brand in this PR. I noted some pain points in the process and creating this issue as a result. The primary paint point involved needing to override this css targeting the image:

.River__visual img,
.River__visual video {
  width: 100%;
  height: 100%;
  object-fit: cover;
}

Issues:

Problem with CSS Specificity:

  • When using multiple images, a user will want to override width: 100%, etc.
  • The Primer Stylelint defines:
'selector-max-type': 0,
'selector-no-qualifying-type': true,
  • But React Brand breaks that rule.
  • This means the only way to override this rule is with !important

Potential Remedies

  • Change the stylelint specification (recommended)
  • Update the CSS to follow the stylelint

No className prop:

  • I believe River.Content & River.Visual should accept a className prop. This can be addressed in a PR for Suggested new default props #42 if you agree.
  • In conjunction with no className prop, only Text, Heading and Link are allowed in River.Content, so there isn't an option to include an internal wrapper to target (e.g. wrapper Header and Text in div).

Thoughts on align prop

  • What it does: Alignment of text content relative to the Visual position
  • How I would expect alignment to be handled: Order of props. e.g.
    • Instead of align="left"
    <River.Content>...</River.Content>
    <River.Visual>...</River.Visual>
    
    • Instead of align="right"
    <River.Visual>...</River.Visual>
    <River.Content>...</River.Content>
    
  • This feels as though this is more in line with the "composability"
  • Could include a boolean center prop.
  • These are just thoughts and not strongly held beliefs haha
@rezrah
Copy link
Collaborator

rezrah commented Jul 18, 2022

Thanks @rfearing.. on the specificity issue...

We should...

  • Enable our primer linters for CSS modules
  • Convert the img and video default selector to a class name, which will enable normal overriding

On align prop, I agree it would feel like a more composable API, but there are reasons why it's this way. Having an align prop will help us infer reading direction for localisation. The DOM nodes themselves are also ordered using this mechanism. Also in a bid to 'sync' our React API to the Figma component, where we need to have an 'align' prop.

If this prop isn't causing major issues right now, I'd recommend we add it to the backlog as a separate Issue so we can circle back to it at the end of the quarter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants