Skip to content

Commit

Permalink
Merge pull request #90 from hypothesis/prefix-component-classnames
Browse files Browse the repository at this point in the history
Add prefixes to all component class names to prevent collisions
  • Loading branch information
lyzadanger authored Jun 8, 2021
2 parents b394593 + aac81bf commit b99cb4e
Show file tree
Hide file tree
Showing 15 changed files with 56 additions and 58 deletions.
12 changes: 6 additions & 6 deletions src/components/Dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ export function Dialog({
<div
aria-labelledby={dialogTitleId}
className={classnames(
'Dialog',
{ 'Dialog--closeable': !!onCancel },
'Hyp-Dialog',
{ 'Hyp-Dialog--closeable': !!onCancel },
contentClass
)}
ref={rootEl}
Expand All @@ -112,21 +112,21 @@ export function Dialog({
>
<header>
{icon && (
<div className="Dialog__header-icon">
<div className="Hyp-Dialog__header-icon">
<SvgIcon name={icon} title={title} data-testid="header-icon" />
</div>
)}
<h2 className="Dialog__title" id={dialogTitleId}>
<h2 className="Hyp-Dialog__title" id={dialogTitleId}>
{title}
</h2>
{onCancel && (
<div className="Dialog__close">
<div className="Hyp-Dialog__close">
<IconButton icon="cancel" title="Close" onClick={onCancel} />
</div>
)}
</header>
<div>{children}</div>
<div className="Dialog__actions">
<div className="Hyp-Dialog__actions">
{onCancel && (
<LabeledButton data-testid="cancel-button" onClick={onCancel}>
{cancelLabel}
Expand Down
4 changes: 2 additions & 2 deletions src/components/Modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ export function Modal({ children, onCancel, ...restProps }) {

return (
<>
<div className="Modal__overlay" />
<div className="Modal" ref={modalContainerRef}>
<div className="Hyp-Modal__overlay" />
<div className="Hyp-Modal" ref={modalContainerRef}>
<Dialog onCancel={onCancel} {...restProps}>
{children}
</Dialog>
Expand Down
10 changes: 6 additions & 4 deletions src/components/Panel.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,19 @@ export function Panel({ children, icon, onClose, title }) {
const withCloseButton = !!onClose;
return (
<div
className={classnames('Panel', { 'Panel--closeable': withCloseButton })}
className={classnames('Hyp-Panel', {
'Hyp-Panel--closeable': withCloseButton,
})}
>
<header>
{icon && (
<div className="Panel__header-icon">
<div className="Hyp-Panel__header-icon">
<SvgIcon name={icon} title={title} />
</div>
)}
<h2 className="Panel__title">{title}</h2>
<h2 className="Hyp-Panel__title">{title}</h2>
{withCloseButton && (
<div className="Panel__close">
<div className="Hyp-Panel__close">
<IconButton icon="cancel" title="Close" onClick={onClose} />
</div>
)}
Expand Down
2 changes: 1 addition & 1 deletion src/components/SvgIcon.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export function SvgIcon({ name, className = '', inline = false, title = '' }) {

return (
<span
className={classnames('SvgIcon', { 'SvgIcon--inline': inline })}
className={classnames('Hyp-SvgIcon', { 'Hyp-SvgIcon--inline': inline })}
dangerouslySetInnerHTML={{ __html: markup }}
ref={element}
{...spanProps}
Expand Down
6 changes: 3 additions & 3 deletions src/components/buttons.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ function ButtonBase({
*
* @param {IconButtonProps} props
*/
export function IconButton({ className = 'IconButton', ...restProps }) {
export function IconButton({ className = 'Hyp-IconButton', ...restProps }) {
const { icon } = restProps;
return (
<ButtonBase className={className} {...restProps}>
Expand All @@ -103,7 +103,7 @@ export function IconButton({ className = 'IconButton', ...restProps }) {
*/
export function LabeledButton({
children,
className = 'LabeledButton',
className = 'Hyp-LabeledButton',
...restProps
}) {
const { icon, iconPosition = 'left' } = restProps;
Expand All @@ -122,5 +122,5 @@ export function LabeledButton({
* @param {ButtonBaseProps} props
*/
export function LinkButton(props) {
return <ButtonBase className="LinkButton" {...props} />;
return <ButtonBase className="Hyp-LinkButton" {...props} />;
}
2 changes: 1 addition & 1 deletion src/components/test/Dialog-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('Dialog', () => {
<span>content</span>
</Dialog>
);
assert.isTrue(wrapper.find('.Dialog').hasClass('foo'));
assert.isTrue(wrapper.find('.Hyp-Dialog').hasClass('foo'));
});

it('renders buttons', () => {
Expand Down
2 changes: 1 addition & 1 deletion src/components/test/Panel-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('Panel', () => {

it('renders the provided title', () => {
const wrapper = createPanel({ title: 'My Panel' });
const titleEl = wrapper.find('.Panel__title');
const titleEl = wrapper.find('.Hyp-Panel__title');
assert.equal(titleEl.text(), 'My Panel');
});

Expand Down
8 changes: 4 additions & 4 deletions src/components/test/SvgIcon-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,16 @@ describe('SvgIcon', () => {
const container = document.createElement('div');
render(<SvgIcon name="arrow-left" />, container);
const wrapper = container.querySelector('span');
assert.isTrue(wrapper.classList.contains('SvgIcon'));
assert.isFalse(wrapper.classList.contains('SvgIcon--inline'));
assert.isTrue(wrapper.classList.contains('Hyp-SvgIcon'));
assert.isFalse(wrapper.classList.contains('Hyp-SvgIcon--inline'));
});

it('appends an inline class to wrapper if `inline` prop is `true`', () => {
const container = document.createElement('div');
render(<SvgIcon name="arrow-left" inline={true} />, container);
const wrapper = container.querySelector('span');
assert.isTrue(wrapper.classList.contains('SvgIcon'));
assert.isTrue(wrapper.classList.contains('SvgIcon--inline'));
assert.isTrue(wrapper.classList.contains('Hyp-SvgIcon'));
assert.isTrue(wrapper.classList.contains('Hyp-SvgIcon--inline'));
});

it('sets a title to the containing `span` element if `title` is present', () => {
Expand Down
23 changes: 9 additions & 14 deletions src/components/test/buttons-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import mockImportedComponents from '../../../test/util/mock-imported-components'

// Add common tests for a button component for stuff provided by `ButtonBase`
function addCommonTests({ componentName, createComponentFn, withIcon = true }) {
const className = `Hyp-${componentName}`;
describe(`${componentName} common support`, () => {
if (withIcon) {
it('renders the indicated icon', () => {
Expand All @@ -17,7 +18,7 @@ function addCommonTests({ componentName, createComponentFn, withIcon = true }) {
const icon = wrapper.find('SvgIcon');
assert.equal(icon.prop('name'), 'fakeIcon');
// Icon is positioned "left" even if it is the only element in the <button>
assert.isTrue(button.hasClass(`${componentName}--icon-left`));
assert.isTrue(button.hasClass(`${className}--icon-left`));
});
}

Expand Down Expand Up @@ -53,43 +54,37 @@ function addCommonTests({ componentName, createComponentFn, withIcon = true }) {
it('uses a default className', () => {
const wrapper = createComponentFn();

assert.isTrue(wrapper.find('button').hasClass(componentName));
assert.isTrue(wrapper.find('button').hasClass(className));
});

['primary', 'light', 'dark'].forEach(variant => {
it('renders a valid variant', () => {
const wrapper = createComponentFn({ variant });

assert.isTrue(
wrapper.find('button').hasClass(`${componentName}--${variant}`)
wrapper.find('button').hasClass(`${className}--${variant}`)
);
});
});

it('sets a `normal` variant modifier class by default', () => {
const wrapper = createComponentFn();

assert.isTrue(
wrapper.find('button').hasClass(`${componentName}--normal`)
);
assert.isTrue(wrapper.find('button').hasClass(`${className}--normal`));
});

['small', 'medium', 'large'].forEach(size => {
it('renders a valid size', () => {
const wrapper = createComponentFn({ size });

assert.isTrue(
wrapper.find('button').hasClass(`${componentName}--${size}`)
);
assert.isTrue(wrapper.find('button').hasClass(`${className}--${size}`));
});
});

it('sets a `medium` size modifier class by default', () => {
const wrapper = createComponentFn();

assert.isTrue(
wrapper.find('button').hasClass(`${componentName}--medium`)
);
assert.isTrue(wrapper.find('button').hasClass(`${className}--medium`));
});

it('overrides className when provided', () => {
Expand All @@ -102,7 +97,7 @@ function addCommonTests({ componentName, createComponentFn, withIcon = true }) {
assert.isTrue(
wrapper.find('button').hasClass('CustomClassName--primary')
);
assert.isFalse(wrapper.find('button').hasClass(componentName));
assert.isFalse(wrapper.find('button').hasClass(className));
});

it('adds inline styles when provided', () => {
Expand Down Expand Up @@ -245,7 +240,7 @@ describe('buttons', () => {
const icon = wrapper.find('SvgIcon');
const button = wrapper.find('button');
assert.equal(icon.prop('name'), 'fakeIcon');
assert.isTrue(button.hasClass('LabeledButton--icon-right'));
assert.isTrue(button.hasClass('Hyp-LabeledButton--icon-right'));
});

it('does not render an icon if none indicated', () => {
Expand Down
29 changes: 15 additions & 14 deletions styles/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,14 @@ Mixins in `mixins` and utility styles in `util` loosely apply [Atomic Design](ht

The directory that a SASS module lives in dictates what it should provide (styles, mixins, variables, etc.) and what kind of dependencies it is allowed:

| Directory | Description | Provides | Dependencies |
| ------------ | ---------------------------- | --------- | --------------------- |
| `base` | Reset and element styles | styles | mixins, variables[^1] |
| `components` | Styles for shared components | styles | any |
| `mixins` | Mixins | mixins | mixins[^2], variables |
| `patterns` | Pattern styles | styles | mixins |
| `util` | Utility styles | styles | mixins |
| `variables` | Variables | variables | none |
| Directory | Description | Provides | Dependencies |
| ------------ | ----------------------------------------------------------- | --------- | --------------------- |
| `base` | Reset and element styles. Used only by the pattern library. | styles | mixins, variables[^1] |
| `components` | Styles for shared components | styles | any |
| `mixins` | Mixins | mixins | mixins[^2], variables |
| `patterns` | Pattern styles | styles | mixins |
| `util` | Utility styles | styles | mixins |
| `variables` | Variables | variables | none |

## CSS Output and Ordering

Expand All @@ -85,13 +85,14 @@ Note `utils` in last position, to assure these classes override other rules.

### Selectors and Class naming

We use [BEM (Block Element Modifier)](http://getbem.com/) methodology for class naming.
All class-name selectors generated by this package's SASS must be prefixed (see details below). We use [BEM (Block Element Modifier)](http://getbem.com/) methodology for class naming.

- `base` modules should use element selectors (no class names).
- Component class names should use PascalCase, e.g. `.HelpPanel`. All other classnames require a .`hyp-` prefix.
- All utility and pattern classnames should be prefixed with `.hyp-`.
- Atomic-level utility classes should be prefixed with `.hyp-u-`, e.g. `.hyp-u-border--left`. These are classes that may be used additively to adjust styling on a single element.
- Composite (molecule, organism) class names do not require `u`-prefixes, but should be lower-case, e.g. `.hyp-frame`.
_Note_: `base` modules use element selectors (no class names). These element selectors and resets are used by the pattern library internally and are not currently part of the public API.

- Component class names should be prefixed with `.Hyp-` and use PascalCase, e.g. `.Hyp-SomePanel`.
- All other class names should be kebab-case:
- Pattern (module, organism, etc.) class names should be prefixed with `.hyp-`, e.g. `.hyp-card`.
- Utility class names should be prefixed with `.hyp-u-`, e.g. `.hyp-u-border--left`. These are classes that may be used individually to adjust styling on a single element.

### Variables

Expand Down
2 changes: 1 addition & 1 deletion styles/components/Dialog.scss
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
@use '../mixins/patterns/organisms';

.Dialog {
.Hyp-Dialog {
@include organisms.panel;
}
4 changes: 2 additions & 2 deletions styles/components/Modal.scss
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
@use '../mixins/layout';
@use '../mixins/patterns/molecules';

.Modal__overlay {
.Hyp-Modal__overlay {
@include layout.overlay;
}

.Modal {
.Hyp-Modal {
@include molecules.modal;
}
2 changes: 1 addition & 1 deletion styles/components/Panel.scss
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
@use '../mixins/patterns/organisms';

.Panel {
.Hyp-Panel {
@include organisms.panel;
}
2 changes: 1 addition & 1 deletion styles/components/SvgIcon.scss
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* Make the wrapper element's size match the contained `svg` element */
.SvgIcon {
.Hyp-SvgIcon {
display: flex;

&--inline {
Expand Down
6 changes: 3 additions & 3 deletions styles/components/buttons/_styles.scss
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
@use 'mixins';

// A button with text, and optionally an icon
.LabeledButton {
.Hyp-LabeledButton {
@include mixins.LabeledButton;
}

// A button with only an icon and no label/text
.IconButton {
.Hyp-IconButton {
@include mixins.IconButton;
}

// A button styled to appear as a link, with underline on hover
.LinkButton {
.Hyp-LinkButton {
@include mixins.LinkButton;
}

0 comments on commit b99cb4e

Please sign in to comment.