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

Jump to bottom

[Components] Update FontSize control #35395

Merged
merged 13 commits into from
Oct 14, 2021
Merged

[Components] Update FontSize control #35395

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f0732b4
temp
ntsekouras Oct 6, 2021
b96d06d
show custom control on `custom` select option + show hints only if is…
ntsekouras Oct 7, 2021
13c98e5
switch conditionally to ToggleGroupControl
ntsekouras Oct 8, 2021
9b2e02d
add story part 1
ntsekouras Oct 8, 2021
e7c292e
Update comments.
mtias Oct 8, 2021
e666227
fix styles + rename var
ntsekouras Oct 12, 2021
b82385d
update README
ntsekouras Oct 13, 2021
4c3b875
small fixes and update unit tests
ntsekouras Oct 13, 2021
fe2e844
update e2e tests part 1
ntsekouras Oct 13, 2021
0961ee0
update current e2e tests + add aria label to toggle group button
ntsekouras Oct 14, 2021
e123e7a
update toggle custom size label
ntsekouras Oct 14, 2021
6be9517
add tests + reorganise them
ntsekouras Oct 14, 2021
4ed3f06
Polish icon size and position, polish font size in dropdown.
jasmussen Oct 14, 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: 9 additions & 8 deletions packages/components/src/custom-select-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { __, sprintf } from '@wordpress/i18n';
*/
import { Button, VisuallyHidden } from '../';

const itemToString = ( item ) => item && item.name;
const itemToString = ( item ) => item?.name;
// This is needed so that in Windows, where
// the menu does not necessarily open on
// key up/down, you can still switch between
Expand Down Expand Up @@ -98,14 +98,9 @@ export default function CustomSelectControl( {
className: 'components-custom-select-control__menu',
'aria-hidden': ! isOpen,
} );
// We need this here, because the null active descendant is not
// fully ARIA compliant.
// We need this here, because the null active descendant is not fully ARIA compliant.
if (
menuProps[ 'aria-activedescendant' ] &&
menuProps[ 'aria-activedescendant' ].slice(
0,
'downshift-null'.length
) === 'downshift-null'
menuProps[ 'aria-activedescendant' ]?.startsWith( 'downshift-null' )
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
) {
delete menuProps[ 'aria-activedescendant' ];
}
Expand Down Expand Up @@ -161,12 +156,18 @@ export default function CustomSelectControl( {
{
'is-highlighted':
index === highlightedIndex,
'has-hint': !! item.hint,
}
),
style: item.style,
} ) }
>
{ item.name }
{ item.hint && (
<span className="components-custom-select-control__item-hint">
{ item.hint }
</span>
) }
Comment on lines +166 to +170
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 it would be better to split changes to CustomSelectControl in a separate PR, where the relevant documentation is also updated and a CHANGELOG entry is added.

The current PR will keep focusing on the FontSizePicker, making changes more contained and easier to find in the future.

Separately from this, I also wonder if we should rewrite the CustomSelectControl component to use Emotion for styling and rewrite it to TypeScript. These improvements would ease the integration of these components with the Typography panel and give more confidence to our refactors (via TYpeScript's static linting)

{ item === selectedItem && (
<Icon
icon={ check }
Expand Down
14 changes: 10 additions & 4 deletions packages/components/src/custom-select-control/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,19 +58,25 @@

.components-custom-select-control__item {
align-items: center;
display: flex;
display: grid;
grid-template-columns: auto auto;
list-style-type: none;
padding: $grid-unit-10;
cursor: default;
line-height: $icon-size + $grid-unit-05;


&.has-hint {
grid-template-columns: auto auto 30px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we handle this better? 🤔 --cc @jasmussen

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered using the Grid component instead of using CSS styles?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand on what I should look for here? Happy to help if I can.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean was to refactor this component to use internally the Grid component — instead of using custom CSS rules for the grid layout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make the minimum changes in CustomSelectControl and I'm not sure there are some better rules I should use - or even maybe flex? Maybe I'll consider splitting these changes to a separate PR as Marco suggested..

}
&.is-highlighted {
background: $gray-300;
}

.components-custom-select-control__item-hint {
font-weight: 200;
text-align: right;
padding-right: $grid-unit-05;
}
.components-custom-select-control__item-icon {
margin-right: 0;
margin-left: auto;
}

Expand Down
9 changes: 9 additions & 0 deletions packages/components/src/font-size-picker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,12 @@ If `true`, the UI will contain a slider, instead of a numeric text input field.
- Type: `Boolean`
- Required: no
- Default: `false`

### withReset

If `true`, a reset button will be displayed alongside the predefined and custom
font size fields.

- Type: `Boolean`
- Required: no
- Default: `true`
Loading