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 reusable preferences modal to interface package. #39153

Merged
merged 8 commits into from
Mar 8, 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
4 changes: 4 additions & 0 deletions packages/interface/src/components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@ export { default as PinnedItems } from './pinned-items';
export { default as MoreMenuDropdown } from './more-menu-dropdown';
export { default as MoreMenuFeatureToggle } from './more-menu-feature-toggle';
export { default as ActionItem } from './action-item';
export { default as PreferencesModal } from './preferences-modal';
export { default as PreferencesModalTabs } from './preferences-modal-tabs';
export { default as PreferencesModalSection } from './preferences-modal-section';
export { default as ___unstablePreferencesModalBaseOption } from './preferences-modal-base-option';
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#__unstablePreferencesModalBaseOption

`__unstablePreferencesModalBaseOption` renders a toggle meant to be used with `PreferencesModal`.

This component implements a `ToggleControl` component from the `@wordpress/components` package.

**It is an unstable component so is subject to breaking changes at any moment. Use at own risk.**

## Example

```jsx
function MyEditorPreferencesOption() {
return (
<__unstablePreferencesModalBaseOption
label={ label }
isChecked={ isChecked }
onChange={ setIsChecked }
>
{ isChecked !== areCustomFieldsEnabled && (
<CustomFieldsConfirmation willEnable={ isChecked } />
) }
</__unstablePreferencesModalBaseOption>
)
}
```

## Props

### help
### label
### isChecked
### onChange

These props are passed directly to ToggleControl, so see [ToggleControl readme](https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/toggle-control/README.md) for more info.

### children

Components to be rendered as content.

- Type: `Element`
- Required: No.

Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* WordPress dependencies
*/
import { ToggleControl } from '@wordpress/components';

function BaseOption( { help, label, isChecked, onChange, children } ) {
return (
<div className="interface-preferences-modal__option">
<ToggleControl
Copy link
Contributor

Choose a reason for hiding this comment

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

Once #39158 is merged, a next step will be to add a PreferencesToggleControl to the preferences package (similar to the way PreferencesToggleMenuItem works).

I'm not sure what the best way to integrate it with this generic toggle control will be 🤔

The preferences package could use this component, but then that becomes only useful in the preferences modal (and I don't think preferences should depend on interface).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason to have this wrapper around ToggleControl is to allow children to be attached to it. If we implement a PreferencesToggleControl with that capacity, we don't need this component at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should start with this one being __unstable and then see how the other toggle component evolves.

help={ help }
label={ label }
checked={ isChecked }
onChange={ onChange }
/>
{ children }
</div>
);
}

export default BaseOption;
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
.interface-preferences-modal__option {
.components-base-control {
.components-base-control__field {
align-items: center;
display: flex;
margin-bottom: 0;

& > label {
flex-grow: 1;
padding: 0.6rem 0 0.6rem 10px;
}
}
}

.components-base-control__help {
margin: -$grid-unit-10 0 $grid-unit-10 58px;
font-size: $helptext-font-size;
font-style: normal;
color: $gray-700;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@

`PreferencesModalSection` renders a section (as a fieldset) meant to be used with `PreferencesModal`.

## Example

See the `PreferencesModal` readme for usage info.


## Props

### title

The title of the section

- Type: `String`
- Required: Yes.

### description

The description for the section.

- Type: `String`
- Required: No.

### children

Components to be rendered as content.

- Type: `Element`
- Required: Yes.
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
const Section = ( { description, title, children } ) => (
<fieldset className="interface-preferences-modal__section">
<legend>
<h2 className="interface-preferences-modal__section-title">
{ title }
</h2>
{ description && (
<p className="interface-preferences-modal__section-description">
{ description }
</p>
) }
</legend>
{ children }
</fieldset>
);

export default Section;
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
.interface-preferences-modal__section {
margin: 0 0 2.5rem 0;

&:last-child {
margin: 0;
}
}

.interface-preferences-modal__section-title {
font-size: 0.9rem;
font-weight: 600;
margin-top: 0;
}

.interface-preferences-modal__section-description {
margin: -$grid-unit-10 0 $grid-unit-10 0;
font-size: $helptext-font-size;
font-style: normal;
color: $gray-700;
}
14 changes: 14 additions & 0 deletions packages/interface/src/components/preferences-modal-tabs/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# PreferencesModalTabs

`PreferencesModalTabs` creates a tabbed interface meant to be used inside a `PreferencesModal`. Markup differs between small and large viewports; on small the tabs are closed by default.

## Example

See the `PreferencesModal` readme for usage info.
## Props
### sections

Sections to populate the modal with. Takes an array of objects, where each should include `name`, `tablabel` and `content`.

- Type: `Array`
- Required: Yes.
156 changes: 156 additions & 0 deletions packages/interface/src/components/preferences-modal-tabs/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/**
* WordPress dependencies
*/
import { useViewportMatch } from '@wordpress/compose';
import {
__experimentalNavigatorProvider as NavigatorProvider,
__experimentalNavigatorScreen as NavigatorScreen,
__experimentalNavigatorButton as NavigatorButton,
__experimentalNavigatorBackButton as NavigatorBackButton,
__experimentalItemGroup as ItemGroup,
__experimentalItem as Item,
__experimentalHStack as HStack,
__experimentalText as Text,
__experimentalTruncate as Truncate,
FlexItem,
TabPanel,
Card,
CardHeader,
CardBody,
} from '@wordpress/components';
import { useMemo, useCallback, useState } from '@wordpress/element';
import { chevronLeft, chevronRight, Icon } from '@wordpress/icons';
import { isRTL, __ } from '@wordpress/i18n';

const PREFERENCES_MENU = 'preferences-menu';

export default function PreferencesModalTabs( { sections } ) {
const isLargeViewport = useViewportMatch( 'medium' );

// This is also used to sync the two different rendered components
// between small and large viewports.
const [ activeMenu, setActiveMenu ] = useState( PREFERENCES_MENU );
/**
* Create helper objects from `sections` for easier data handling.
* `tabs` is used for creating the `TabPanel` and `sectionsContentMap`
* is used for easier access to active tab's content.
*/
const { tabs, sectionsContentMap } = useMemo( () => {
let mappedTabs = {
tabs: [],
sectionsContentMap: {},
};
if ( sections.length ) {
mappedTabs = sections.reduce(
( accumulator, { name, tabLabel: title, content } ) => {
accumulator.tabs.push( { name, title } );
accumulator.sectionsContentMap[ name ] = content;
return accumulator;
},
{ tabs: [], sectionsContentMap: {} }
);
}
return mappedTabs;
}, [ sections ] );

const getCurrentTab = useCallback(
( tab ) => sectionsContentMap[ tab.name ] || null,
[ sectionsContentMap ]
);

let modalContent;
// We render different components based on the viewport size.
if ( isLargeViewport ) {
modalContent = (
<TabPanel
className="interface-preferences__tabs"
tabs={ tabs }
initialTabName={
activeMenu !== PREFERENCES_MENU ? activeMenu : undefined
}
onSelect={ setActiveMenu }
orientation="vertical"
>
{ getCurrentTab }
</TabPanel>
);
} else {
modalContent = (
<NavigatorProvider
initialPath="/"
className="interface-preferences__provider"
>
<NavigatorScreen path="/">
<Card isBorderless size="small">
<CardBody>
<ItemGroup>
{ tabs.map( ( tab ) => {
return (
<NavigatorButton
key={ tab.name }
path={ tab.name }
as={ Item }
isAction
>
<HStack justify="space-between">
<FlexItem>
<Truncate>
{ tab.title }
</Truncate>
</FlexItem>
<FlexItem>
<Icon
icon={
isRTL()
? chevronLeft
: chevronRight
}
/>
</FlexItem>
</HStack>
</NavigatorButton>
);
} ) }
</ItemGroup>
</CardBody>
</Card>
</NavigatorScreen>
{ sections.length &&
sections.map( ( section ) => {
return (
<NavigatorScreen
key={ `${ section.name }-menu` }
path={ section.name }
>
<Card isBorderless size="large">
<CardHeader
isBorderless={ false }
justify="left"
size="small"
gap="6"
>
<NavigatorBackButton
icon={
isRTL()
? chevronRight
: chevronLeft
}
aria-label={ __(
'Navigate to the previous view'
) }
/>
<Text size="16">
{ section.tabLabel }
</Text>
</CardHeader>
<CardBody>{ section.content }</CardBody>
</Card>
</NavigatorScreen>
);
} ) }
</NavigatorProvider>
);
}

return modalContent;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
$vertical-tabs-width: 160px;

.interface-preferences__tabs {
.components-tab-panel__tabs {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should try to avoid using internal components classnames (as they are not regarded as "public APIs") and to override styles this way, as those styles can easily break in case the component is updated in the future.

I understand this may also be caused by a limitation in TabPanel's design, or by its misalignment with the recent UI direction — cc'ing @jasmussen here who may have more insights here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback @ciampo !

These styles were copied verbatim from the original edit-post modal, so they've been around for a while now. For components-tab-panel__tabs and components-modal__content I could potentially target them by aria-role instead - would this be preferable?

The problem is components-base-control__help is a p tag with no distinctive features, so there's no other way to target it. (By the way: that's an accessibility issue, because the contents of that tag are ignored by screen readers. It should be linked to the control through an aria-describedby or similar. I'm happy to create an issue for that if there isn't one already 🙂 )

In more general terms, are there any thoughts around expanding the components API to allow applying styles to child elements of a component? It's quite frequent across the codebase that we resort to the internal classnames for this end. It would be good to find a solution, e.g.:

  • make some of those classnames an official part of the API;
  • provide a way to pass classnames for child elements (this can become messy though)
  • provide a CSS custom property API whereby consumers can set certain values for the child elements (colors, spacing etc.) of a component.

Let me know what you think!

Copy link
Contributor

@ciampo ciampo Mar 7, 2022

Choose a reason for hiding this comment

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

These styles were copied verbatim from the original edit-post modal, so they've been around for a while now.

Thank you for the explanation, I previously had missed this detail (my fault, I skipped the PR's description and went straight to the ping from Riad). I'd say, let's try to see if there's any low-hanging fruit (like this comment) and leave more complicated refactors for another time!

For components-tab-panel__tabs and components-modal__content I could potentially target them by aria-role instead - would this be preferable?

That would be slightly better, but we can probably leave it as-is in this PR. I would be personally questioning why we need to override so many styles for the Modal and TabPanel components — the better solution in my opinion would be to avoid these styles overrides as much as possible. In a follow-up PR we could look at what limitations we're facing when using these components, and see how we can overcome them (by either improving these components or using a different set of components).

The problem is components-base-control__help is a p tag with no distinctive features, so there's no other way to target it. (By the way: that's an accessibility issue, because the contents of that tag are ignored by screen readers. It should be linked to the control through an aria-describedby or similar. I'm happy to create an issue for that if there isn't one already 🙂 )

Understood — feel free to create an issue for that, and tag @mirka and me.

In more general terms, are there any thoughts around expanding the components API to allow applying styles to child elements of a component? It's quite frequent across the codebase that we resort to the internal classnames for this end. It would be good to find a solution, e.g.:

  • make some of those classnames an official part of the API;
  • provide a way to pass classnames for child elements (this can become messy though)
  • provide a CSS custom property API whereby consumers can set certain values for the child elements (colors, spacing etc.) of a component.

On this topic, we think that hardcoded classnames and internal DOM structure should not be part of the public APIs of components, as they would severely limit the amount of internal (private) changes that can be made to the components; most internal changes would likely cause breakages for the consumers of such components.

I also agree that exposing more props for children classnames is not the correct solution, as it would scale very poorly.

Regarding CSS custom properties, it's something that we're going to take a look at soon when we'll start actively working on introducing a lightweight theming API layer.

Another approach that we could have is to try and be more "modular" with our components — e.g. expose a BaseControl and a BaseControlHelp components that the consumer of the library would need to use together (rather than exporting one monolithic BaseControl component).

cc @mirka

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've created #39271 to track the issue with the help text. I also fixed the CSS targeting components-navigator-provider so I think that's all your feedback addressed. Thanks @ciampo !

position: absolute;
top: $header-height + $grid-unit-30;
// Aligns button text instead of button box.
left: $grid-unit-20;
width: $vertical-tabs-width;
.components-tab-panel__tabs-item {
border-radius: $radius-block-ui;
font-weight: 400;
&.is-active {
background: $gray-100;
box-shadow: none;
font-weight: 500;
}
&:focus:not(:disabled) {
box-shadow: inset 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color);
}
}
}
.components-tab-panel__tab-content {
padding-left: $grid-unit-30;
margin-left: $vertical-tabs-width;
}
}

@media (max-width: #{ ($break-medium - 1) }) {
// Keep the navigator component from overflowing the modal content area
// to ensure that sticky position elements stick where intended.
.interface-preferences__provider {
height: 100%;
}
}
Loading