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 AdderToolbar components and styling #4753

Merged
merged 3 commits into from
Aug 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"@babel/preset-react": "^7.0.0",
"@babel/preset-typescript": "^7.16.7",
"@hypothesis/frontend-build": "1.2.0",
"@hypothesis/frontend-shared": "5.4.0",
"@hypothesis/frontend-shared": "5.4.1",
"@octokit/rest": "^19.0.3",
"@rollup/plugin-babel": "^5.3.0",
"@rollup/plugin-commonjs": "^22.0.0",
Expand Down
98 changes: 61 additions & 37 deletions src/annotator/components/AdderToolbar.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
import classnames from 'classnames';
import { LabeledButton, Icon } from '@hypothesis/frontend-shared';
import {
AnnotateIcon,
ButtonBase,
HighlightIcon,
PointerDownIcon,
PointerUpIcon,
} from '@hypothesis/frontend-shared/lib/next';

import { useShortcut } from '../../shared/shortcut';

/**
* @typedef {import('@hypothesis/frontend-shared/lib/types').IconComponent} IconComponent
*/

/**
* Render an inverted light-on-dark "pill" with the given `badgeCount`
* (annotation count). This is rendered instead of an icon on the toolbar
Expand All @@ -16,11 +26,12 @@ function NumberIcon({ badgeCount }) {
<span
className={classnames(
'rounded px-1 py-0.5',
'text-color-text-inverted font-bold bg-grey-7',
'dim-bg'
// The background color is inherited from the current text color in
// the containing button and will vary depending on hover state.
'bg-current'
)}
>
{badgeCount}
<span className="font-bold text-color-text-inverted">{badgeCount}</span>
</span>
);
}
Expand All @@ -34,55 +45,58 @@ function NumberIcon({ badgeCount }) {
*/
function AdderToolbarArrow({ arrowDirection }) {
return (
<Icon
name="pointer"
classes={classnames(
// Position the arrow in the horizontal center at the bottom of the
// container (toolbar). Note: the arrow is pointing up at this point.
'absolute left-1/2 -translate-x-1/2',
// Override `1em` width/height rules in `Icon` to size the arrow as
// its SVG dimensions dictate
'h-auto w-auto z-2',
'text-grey-3 fill-white',
<div
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an example of me adding an additional parent component to handle the positioning, and not applying that styling directly to the icon component. This is less of a real concern with icons, which have no classes applied to them other than what you set on className, but it's more hygienic IMHO.

className={classnames(
// Position horizontally center of the AdderToolbar
'absolute left-1/2 -translate-x-1/2 z-2',
'fill-white text-grey-3',
{
// Down arrow: transform to point the arrow down
'rotate-180': arrowDirection === 'down',
// Up arrow: position vertically above the toolbar
// Move the pointer to the top of the AdderToolbar
'top-0 -translate-y-full': arrowDirection === 'up',
}
)}
/>
>
{arrowDirection === 'up' ? <PointerUpIcon /> : <PointerDownIcon />}
</div>
);
}

/**
* @param {object} props
* @param {number} [props.badgeCount]
* @param {string} [props.icon]
* @param {IconComponent} [props.icon]
* @param {string} props.label
* @param {() => void} props.onClick
* @param {string|null} props.shortcut
*/
function ToolbarButton({ badgeCount, icon, label, onClick, shortcut }) {
function ToolbarButton({ badgeCount, icon: Icon, label, onClick, shortcut }) {
useShortcut(shortcut, onClick);

const title = shortcut ? `${label} (${shortcut})` : label;

return (
<LabeledButton
className={classnames(
'flex flex-col gap-y-1 items-center py-2.5 px-2',
'text-annotator-sm leading-none text-grey-7',
'transition-colors duration-200',
'dim-item'
<ButtonBase
classes={classnames(
'flex-col gap-y-1 py-2.5 px-2',
'text-annotator-sm leading-none',
// Default color when the toolbar is not hovered
'text-grey-7',
// When the parent .group element is hovered (but this element itself is
// not), dim this button's text. This has the effect of dimming inactive
// buttons.
'group-hover:text-grey-5',
// When the parent .group element is hovered AND this element is
// hovered, this is the "active" button. Intensify the text color, which
// will also darken any descendant Icon
'hover:group-hover:text-grey-9'
)}
onClick={onClick}
title={title}
>
{icon && <Icon classes="text-annotator-lg" name={icon} title={title} />}
{Icon && <Icon className="text-annotator-lg" title={title} />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least className gives a hint that your classes are the only classes...

{typeof badgeCount === 'number' && <NumberIcon badgeCount={badgeCount} />}
<span className="font-normal">{label}</span>
</LabeledButton>
<span>{label}</span>
</ButtonBase>
);
}

Expand Down Expand Up @@ -135,33 +149,43 @@ export default function AdderToolbar({
return (
<div
className={classnames(
'AdderToolbar',
'absolute select-none bg-white rounded shadow-adder-toolbar',
// Because `.AdderToolbar` rules reset `all:initial`, we cannot use
// default border values from Tailwind and have to be explicit about
// all border attributes
// Reset all inherited properties to their initial values. This prevents
// CSS property values from the host page being inherited by elements of
// the Adder, even when using Shadow DOM.
'all-initial',
// As we've reset all properties to initial values, we cannot rely on
// default border values from Tailwind and have to be explicit about all
// border attributes.
'border border-solid border-grey-3',
'absolute select-none bg-white rounded shadow-adder-toolbar',
// Start at a very low opacity as we're going to fade in in the animation
'opacity-5',
{
'animate-adder-pop-up': arrowDirection === 'up' && isVisible,
'animate-adder-pop-down': arrowDirection === 'down' && isVisible,
}
)}
data-component="AdderToolbar"
dir="ltr"
style={{
visibility: isVisible ? 'visible' : 'hidden',
}}
>
<div className="flex dim-items-on-hover">
<div
className={classnames(
// This group is used to manage hover state styling for descendant
// buttons
'flex group'
)}
>
<ToolbarButton
icon="annotate"
icon={AnnotateIcon}
onClick={() => onCommand('annotate')}
label="Annotate"
shortcut={annotateShortcut}
/>
<ToolbarButton
icon="highlight"
icon={HighlightIcon}
onClick={() => onCommand('highlight')}
label="Highlight"
shortcut={highlightShortcut}
Expand Down
2 changes: 1 addition & 1 deletion src/annotator/test/adder-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ describe('Adder', () => {
it('renders the adder toolbar into a shadow root', () => {
const shadowRoot = getContent(adder);
assert.exists(shadowRoot);
assert.exists(shadowRoot.querySelector('.AdderToolbar'));
assert.exists(shadowRoot.querySelector('[data-component="AdderToolbar"]'));
});

describe('button and shortcut handling', () => {
Expand Down
37 changes: 0 additions & 37 deletions src/styles/annotator/components/AdderToolbar.scss

This file was deleted.

1 change: 0 additions & 1 deletion src/styles/annotator/components/_index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,5 @@
@use '@hypothesis/frontend-shared/styles';

// Annotator-specific components.
@use './AdderToolbar';
@use './Buckets';
@use './sidebar';
10 changes: 9 additions & 1 deletion tailwind.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ export default {
// sub-components to select for that state.
addVariant('sidebar-collapsed', '.sidebar-collapsed &');
}),
plugin(({ addUtilities }) => {
plugin(({ addComponents, addUtilities }) => {
// Tailwind does not provide hyphens-related utility classes.
addUtilities({
'.hyphens-none': {
Expand All @@ -210,6 +210,14 @@ export default {
hyphens: 'auto',
},
});
addComponents({
// Add a custom class to set all properties to initial values. Used
// within shadow DOMs. This must be on the components layer such that it
// gets applied "before" utility classes.
'.all-initial': {
all: 'initial',
},
});
}),
],
};
8 changes: 4 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1110,10 +1110,10 @@
fancy-log "^1.3.3"
glob "^7.2.0"

"@hypothesis/[email protected].0":
version "5.4.0"
resolved "https://registry.yarnpkg.com/@hypothesis/frontend-shared/-/frontend-shared-5.4.0.tgz#60b10bf5d576879176c0355dad883b465d43714d"
integrity sha512-GROm1HUzEto84Mwlix2pUoZfQNLPhqKIn+Y6d1gnwjvDUU2YxUOm+TWYtqmIRCEtEAb5jnm/VUsRJ7DdI8mYlg==
"@hypothesis/[email protected].1":
version "5.4.1"
resolved "https://registry.yarnpkg.com/@hypothesis/frontend-shared/-/frontend-shared-5.4.1.tgz#c0a6ce5c6e8ef9bc5ed5985d57c2bd1ec5f04c2a"
integrity sha512-tM+pypWahpMf2x4tarbhsmw+dQluJUZv8o9CqquVS3FtUucPZ6OJn3Rml6fWMZ0k3DVrDoRMS19VONjtig27Hw==
dependencies:
highlight.js "^11.6.0"

Expand Down