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

Remove the post permalink UI from the post title #21099

Merged
merged 4 commits into from
Mar 26, 2020
Merged

Remove the post permalink UI from the post title #21099

merged 4 commits into from
Mar 26, 2020

Conversation

youknowriad
Copy link
Contributor

closes #5494

This PR removes the Post Permalink UI from the Post Title as suggested in #5494
At the same time, it simplifies the DOM and styles for the post title a lot.

Notes

In 2019, you'll notice a bug in the PostTitle (not centered properly) but I believe this is because the editor styles are targetting these internal classNames while ideally they shouldn't. cc @kjellr

I tried keeping the old classNames to reduce the scope of the breakage and and added "Needs dev note".

@youknowriad youknowriad self-assigned this Mar 24, 2020
@youknowriad youknowriad requested review from jasmussen and kjellr March 24, 2020 09:43
@youknowriad youknowriad added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). General Interface Parts of the UI which don't fall neatly under other labels. [Type] Code Quality Issues or PRs that relate to code quality labels Mar 24, 2020
@github-actions
Copy link

github-actions bot commented Mar 24, 2020 •
edited
Loading

Size Change: -2.25 kB (0%)

Total Size: 857 kB

Filename Size Change
build/autop/index.js 2.58 kB -2 B (0%)
build/block-editor/index.js 102 kB -2 B (0%)
build/block-editor/style-rtl.css 11 kB +3 B (0%)
build/block-editor/style.css 11 kB +4 B (0%)
build/block-library/style-rtl.css 7.43 kB +2 B (0%)
build/block-library/style.css 7.44 kB +1 B
build/block-serialization-default-parser/index.js 1.65 kB -1 B
build/blocks/index.js 57.5 kB +1 B
build/components/index.js 191 kB -9 B (0%)
build/data/index.js 8.25 kB -1 B
build/edit-post/index.js 91.2 kB +17 B (0%)
build/edit-post/style-rtl.css 8.43 kB -38 B (0%)
build/edit-post/style.css 8.43 kB -37 B (0%)
build/edit-site/index.js 6.72 kB +3 B (0%)
build/edit-site/style-rtl.css 2.91 kB +27 B (0%)
build/edit-site/style.css 2.9 kB +24 B (0%)
build/edit-widgets/index.js 4.43 kB +3 B (0%)
build/edit-widgets/style-rtl.css 2.57 kB -12 B (0%)
build/edit-widgets/style.css 2.57 kB -12 B (0%)
build/editor/index.js 42.8 kB -1.01 kB (2%)
build/editor/style-rtl.css 3.38 kB -611 B (18%) 👏
build/editor/style.css 3.38 kB -600 B (17%) 👏
build/i18n/index.js 3.49 kB -1 B
build/keyboard-shortcuts/index.js 2.3 kB -1 B
build/list-reusable-blocks/index.js 2.99 kB +1 B
build/media-utils/index.js 4.84 kB -1 B
build/nux/index.js 3.01 kB -2 B (0%)
build/plugins/index.js 2.54 kB +1 B
build/rich-text/index.js 14.5 kB +2 B (0%)
build/token-list/index.js 1.27 kB +1 B
build/viewport/index.js 1.61 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.23 kB 0 B
build/block-library/index.js 110 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.95 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keycodes/index.js 1.69 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 781 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/url/index.js 4.01 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@jasmussen
Copy link
Contributor

Thanks for this. This is what I see:

Screenshot 2020-03-24 at 11 16 51

I'm very happy to see so much red code. That's good. Removing the permalink UI from the title block is also fine, because:

  • Importantly, it addresses Improve Permalink UI placement and tab order #5494
  • There is duplicate UI in the sidebar
  • Customizing the permalink is sort of an advanced feature
  • It was never that discoverable by the title
  • The title will become a block eventually

I would have liked to see the permalink UI also be in the pre-publish dialog. But this PR doesn't preclude that from happening.

So high level, 👍 👍 on this.

However, there are some issues with the responsive breakpoints:

breakpoints

That's TwentyNineteen, but that theme does not appear to be at fault, as the same issues appear when no editor style is loaded:

vanilla

I have a bit on my plate today, and toddlers to juggle, but if this isn't easy to fix, I can help out maybe tomorrow.

@youknowriad
Copy link
Contributor Author

@jasmussen I'll look at the responsive issues, we might not have the same 2019 styles though as It's always left-aligned for me.

@jasmussen
Copy link
Contributor

Yep that left alignment has been fixed in the RC release but not 5.3

@youknowriad youknowriad requested a review from ellatrix as a code owner March 25, 2020 08:19
@youknowriad
Copy link
Contributor Author

I think I fixed the issue in 4ae1990
The problem is that we had padding around the canvas but it was only applied to the block list, not the post title. I moved that style to a more englobing selector (and tweaked it to adapt for the removal of the side UI)

@gziolo
Copy link
Member

gziolo commented Mar 25, 2020

This PR removes the Post Permalink UI from the Post Title as suggested in #5494
At the same time, it simplifies the DOM and styles for the post title a lot.

It looks like the removed component was never part of the public API, that's good 👍

@jasmussen
Copy link
Contributor

The recent change appears to have messed a bit with the full-width styles, which I believe apply negative margin to account for the padding you removed.

I think it's good to remove that padding, it has to be at some point.

jasmussen
jasmussen approved these changes Mar 25, 2020
View reviewed changes
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

This tests well for me. I tested fullwide images in TwentyTwenty and a theme that doesn't load editor styles, and things behave well. I'm loving how much code we can remove.

TwentyNineteen has an issue with fullwide images, but that appears to be the case beyond this PR. Let's consider creating a trac ticket for it.

@youknowriad youknowriad force-pushed the remove/post-permalink branch from d3b2c36 to df3e79a Compare March 26, 2020 09:20
@youknowriad youknowriad merged commit dc4444d into master Mar 26, 2020
@youknowriad youknowriad deleted the remove/post-permalink branch March 26, 2020 09:57
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Mar 26, 2020
@ellatrix
Copy link
Member

Oh, thanks! This is so much better. 👍

This was referenced Mar 26, 2020
Closed
Closed
@jasmussen jasmussen mentioned this pull request Mar 27, 2020
Merged
@laurelfulford laurelfulford mentioned this pull request Apr 24, 2020
Merged
6 tasks
@Soean Soean mentioned this pull request Aug 15, 2020
Closed
@jsnajdr jsnajdr mentioned this pull request Feb 7, 2022
Merged
@afercia afercia mentioned this pull request Sep 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasmussen jasmussen jasmussen approved these changes

@kjellr kjellr Awaiting requested review from kjellr

@ellatrix ellatrix Awaiting requested review from ellatrix

Assignees

@youknowriad youknowriad

Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). General Interface Parts of the UI which don't fall neatly under other labels. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 7.9
Development

Successfully merging this pull request may close these issues.

Improve Permalink UI placement and tab order
4 participants
@youknowriad @jasmussen @gziolo @ellatrix