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.

Sign up for GitHub

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

LineHeightControl: Use __unstableSize prop in Typography panel #36196

Closed
wants to merge 2 commits into from
Closed

LineHeightControl: Use __unstableSize prop in Typography panel #36196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3666dae
LineHeightControl: Add `__unstableSize="large"` variant
mirka Nov 4, 2021
2ea6c8e
Enable large size in Typography panels
mirka Nov 4, 2021
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
17 changes: 15 additions & 2 deletions packages/block-editor/src/components/line-height-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
Expand All @@ -15,7 +20,11 @@ import {
isLineHeightDefined,
} from './utils';

export default function LineHeightControl( { value: lineHeight, onChange } ) {
export default function LineHeightControl( {
__unstableSize,
value: lineHeight,
onChange,
} ) {
const isDefined = isLineHeightDefined( lineHeight );

const handleOnKeyDown = ( event ) => {
Expand Down Expand Up @@ -64,7 +73,11 @@ export default function LineHeightControl( { value: lineHeight, onChange } ) {
const value = isDefined ? lineHeight : RESET_VALUE;

return (
<div className="block-editor-line-height-control">
<div
className={ classnames( 'block-editor-line-height-control', {
'is-unstable-size-large': __unstableSize === 'large',
} ) }
>
<TextControl
autoComplete="off"
onKeyDown={ handleOnKeyDown }
Expand Down
10 changes: 10 additions & 0 deletions packages/block-editor/src/components/line-height-control/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,14 @@
display: block;
max-width: 60px;
}

// TODO: This may be easier if we can replace TextControl with NumberControl and use prop `size="__unstable-large"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you make a good point here. The fact that this control is also not a NumberControl means you can't drag up/down like most other number related inputs. This catches me out a lot and has also come up in a few team chats and reviews of the typography panel previously.

Quickly switching out the TextControl for the NumberControl appeared to work fine for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that's good to know! I'll want to discuss with @ciampo the implications of using an experimental component (NumberControl) inside a stable component though, I already got caught in that trap once 😆

I added it to #36230 to keep track of the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be definitely up for switching to using NumberControl here — not only if makes sense in terms on semantics and UX, but the code would also look cleaner.

Regarding using experimental components inside stable components, I'm generally ok with that, especially if the experimental component remains as an implementation detail of the stable component, that can be swapped/changed without introducing breaking changes to the public APIs of the stable component.

Alternatively, I'd try to see if we can apply the "size" normalisation to the TextControl component in the @wordpress/components package, rather than doing in in the block editor

&.is-unstable-size-large {
input {
max-width: none;
min-height: 40px;
padding-left: 16px;
padding-right: 16px;
}
}
}
1 change: 1 addition & 0 deletions packages/block-editor/src/hooks/line-height.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export function LineHeightEdit( props ) {
};
return (
<LineHeightControl
__unstableSize="large"
value={ style?.typography?.lineHeight }
onChange={ onChange }
/>
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/tools-panel/stories/typography-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ export const TypographyPanel = () => {
isShownByDefault={ true }
>
<LineHeightControl
__unstableSize="large"
value={ lineHeight }
onChange={ setLineHeight }
/>
Expand Down
1 change: 1 addition & 0 deletions packages/edit-site/src/components/global-styles/typography-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ export default function TypographyPanel( { name } ) {
) }
{ hasLineHeightEnabled && (
<LineHeightControl
__unstableSize="large"
value={ lineHeight }
onChange={ setLineHeight }
/>
Expand Down