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

Add prefixes to all component class names to prevent collisions #90

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

lyzadanger
Copy link
Contributor

Problem:

  • This package's SASS endpoint will generate CSS styles for all components—there is no supported ability for a package user to be selective about which components they want styles for at present[1]
  • This package is increasingly containing generically-named components that have local variants of the same name in applications: Panel, Dialog, Modal.
  • There is a danger (nay, more than a danger, a certainty) that there will be collisions after the next package release unless we take some sort of action with regard to the generated CSS class names.

This PR takes an approach of prefixing all component class names (utility and other classes are already prefixed). Details are in the commit message.

We may choose another tactic for component class-naming, I don't care much, but we need to address this in some way before we cut another release of this package.

Note that this PR is currently dependent on #88 — this is more of a convenience to save time going back and re-doing classnames for the modal components—but I can eliminate that dependency if it becomes troublesome.

[1] Changing this is certainly a viable plan, in the longer term

@lyzadanger lyzadanger requested a review from robertknight June 4, 2021 17:22
@lyzadanger lyzadanger force-pushed the prefix-component-classnames branch from cd20731 to 06fdd3e Compare June 7, 2021 13:46
@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #90 (06fdd3e) into main (6a35284) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 06fdd3e differs from pull request most recent head aac81bf. Consider uploading reports for the commit aac81bf to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##              main       #90   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          123       123           
  Branches        39        39           
=========================================
  Hits           123       123           
Impacted Files Coverage Δ
src/components/SvgIcon.js 100.00% <ø> (ø)
src/components/Dialog.js 100.00% <100.00%> (ø)
src/components/Modal.js 100.00% <100.00%> (ø)
src/components/Panel.js 100.00% <100.00%> (ø)
src/components/buttons.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b394593...aac81bf. Read the comment docs.

Base automatically changed from Modal-component to main June 7, 2021 17:49
Currently, the SASS entrypoint for this package generates all styles for
all shared components. While we may wish to adapt that piece of the
puzzle at some point to give more granular control, some protection is
needed in the short term at the very least to protect against CSS
conflicts for generically-named components like `Panel`, `Modal`,
`Dialog`, etc., for applications that use this package but might not
yet actually use those individual components (i.e. have implementations
of their own at present).

Utility and pattern classes (which are not currently part of the public
API but will and may be, respectively) are already prefixed with
`.hyp-`.

Adhering to the same general naming principle, these changes add a
`.Hyp-` prefix to all component classes, resulting in, e.g.
`.Hyp-Panel`. Use of these classes is currently an internal concern
only, so these changes should have no effect for consuming applications.
Customization of common component styling,  such as it has been done
thus far, uses mixins and doesn't directly reference class names.

This change should also make it easier to spot whether a component is
common or local to the application.
@lyzadanger lyzadanger force-pushed the prefix-component-classnames branch from 06fdd3e to aac81bf Compare June 7, 2021 17:51
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

Looks to me like a straightforward solution to the problem of potential conflicts between this library and applications.

@lyzadanger lyzadanger merged commit b99cb4e into main Jun 8, 2021
@lyzadanger lyzadanger deleted the prefix-component-classnames branch June 8, 2021 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants