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

Use a specific CSS class for the CSS reset mixin. #16856

Merged
merged 3 commits into from
Aug 2, 2019
Merged

Use a specific CSS class for the CSS reset mixin. #16856

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64b005d
Use a specific CSS class for the CSS reset mixin.
afercia Aug 1, 2019
229bca3
List selectors individually instead.
afercia Aug 1, 2019
5a6c5b0
Add selectors for the missing UI parts.
afercia Aug 1, 2019
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 packages/components/src/modal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ class Modal extends Component {
>
<ModalFrame
className={ classnames(
'components-modal__frame',
'components-modal__frame block-editor-styles-reset',
className
) }
onRequestClose={ onRequestClose }
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ const Popover = ( {
const animateXAxis = xAxisMapping[ popoverPosition.xAxis ] || 'center';

const classes = classnames(
'components-popover',
'components-popover block-editor-styles-reset',
className,
'is-' + popoverPosition.yAxis,
'is-' + popoverPosition.xAxis,
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/popover/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ exports[`Popover should pass additional props to portaled element 1`] = `
>
<div>
<div
class="components-popover is-bottom is-center components-animate__appear is-from-top"
class="components-popover block-editor-styles-reset is-bottom is-center components-animate__appear is-from-top"
role="tooltip"
style=""
>
Expand All @@ -30,7 +30,7 @@ exports[`Popover should render content 1`] = `
>
<div>
<div
class="components-popover is-bottom is-center components-animate__appear is-from-top"
class="components-popover block-editor-styles-reset is-bottom is-center components-animate__appear is-from-top"
style=""
>
<div
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-post/src/components/sidebar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const { Fill, Slot } = createSlotFill( 'Sidebar' );
function Sidebar( { children, label, className } ) {
return (
<div
className={ classnames( 'edit-post-sidebar', className ) }
className={ classnames( 'edit-post-sidebar block-editor-styles-reset', className ) }
role="region"
aria-label={ label }
tabIndex="-1"
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-post/src/components/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import PluginBlockSettingsMenuGroup from '../block-settings-menu/plugin-block-se

function VisualEditor() {
return (
<BlockSelectionClearer className="edit-post-visual-editor editor-styles-wrapper">
<BlockSelectionClearer className="edit-post-visual-editor editor-styles-wrapper block-editor-styles-reset">
<VisualEditorGlobalKeyboardShortcuts />
<MultiSelectScrollIntoView />
<WritingFlow>
Expand Down
5 changes: 2 additions & 3 deletions packages/edit-post/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,8 @@ body.block-editor-page {
@include wp-admin-reset( ".block-editor" );
}

.block-editor,
// The modals are shown outside the .block-editor wrapper, they need these styles
.components-modal__frame {
// Target the editor UI but exclude the metaboxes and custom fields areas.
.block-editor-styles-reset {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit concerned about all these reset classes added everywhere. Can't we replace the previous .block-editor with something more specific instead?

Also, ideally we should try to remove that "reset" entirely by moving the styles to the specific components making the components more reusable without relying on global styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

block-editor contains the meta boxes and custom fields area. The only way would be to use a :not(), and I'm not sure if that would be any better. Another approach would be to target each UI section individually, as it worked before. Something like:

.editor-post-permalink,
.edit-post-sidebar,
.editor-post-publish-panel,
.editor-styles-wrapper,
.components-popover,
.components-modal__frame { ... }

but that would produce several lines of CSS selectors, while a single class would produce way less lines. No strong preferences on my side.

Copy link
Contributor Author

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 remove that "reset" entirely by moving the styles to the specific components making the components more reusable without relying on global styles.

I'd agree. A bit out of the scope of this PR though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a preference for the :not approach if it works otherwise listing the selectors is better. That way we avoid introducing a block-editor-styles-reset class in a generic component like "modal". A Modal is agnostic of where it's being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK thanks, will try the :not() approach as soon as I have some time.

@include reset;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/components/post-publish-panel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export class PostPublishPanel extends Component {
const isPrePublish = ! isPublishedOrScheduled && ! isSaving;
const isPostPublish = isPublishedOrScheduled && ! isSaving;
return (
<div className="editor-post-publish-panel" { ...propsForPanel }>
<div className="editor-post-publish-panel block-editor-styles-reset" { ...propsForPanel }>
<div className="editor-post-publish-panel__header">
{ isPostPublish ? (
<div className="editor-post-publish-panel__header-published">
Expand Down
10 changes: 5 additions & 5 deletions packages/editor/src/components/post-publish-panel/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`PostPublishPanel should render the post-publish panel if the post is published 1`] = `
<div
className="editor-post-publish-panel"
className="editor-post-publish-panel block-editor-styles-reset"
>
<div
className="editor-post-publish-panel__header"
Expand Down Expand Up @@ -37,7 +37,7 @@ exports[`PostPublishPanel should render the post-publish panel if the post is pu

exports[`PostPublishPanel should render the post-publish panel if the post is scheduled 1`] = `
<div
className="editor-post-publish-panel"
className="editor-post-publish-panel block-editor-styles-reset"
>
<div
className="editor-post-publish-panel__header"
Expand Down Expand Up @@ -72,7 +72,7 @@ exports[`PostPublishPanel should render the post-publish panel if the post is sc

exports[`PostPublishPanel should render the pre-publish panel if post status is scheduled but date is before now 1`] = `
<div
className="editor-post-publish-panel"
className="editor-post-publish-panel block-editor-styles-reset"
>
<div
className="editor-post-publish-panel__header"
Expand Down Expand Up @@ -111,7 +111,7 @@ exports[`PostPublishPanel should render the pre-publish panel if post status is

exports[`PostPublishPanel should render the pre-publish panel if the post is not saving, published or scheduled 1`] = `
<div
className="editor-post-publish-panel"
className="editor-post-publish-panel block-editor-styles-reset"
>
<div
className="editor-post-publish-panel__header"
Expand Down Expand Up @@ -150,7 +150,7 @@ exports[`PostPublishPanel should render the pre-publish panel if the post is not

exports[`PostPublishPanel should render the spinner if the post is being saved 1`] = `
<div
className="editor-post-publish-panel"
className="editor-post-publish-panel block-editor-styles-reset"
>
<div
className="editor-post-publish-panel__header"
Expand Down