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

Improve the Back to WP button in Fullscreen Mode #20603

Merged
merged 6 commits into from Mar 5, 2020

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Mar 3, 2020

closes #20579

This PR tries to implement the proposal in 120579. It doesn't change the button label because it's CPT specific and we only have the view_items label here https://developer.wordpress.org/reference/functions/get_post_type_labels/ (potentially we could change to a more generic label like "Back to WP Admin"

Capture d’écran 2020-03-03 à 9 42 23 AM

The button does feel a bit prominent but maybe that's fine.

@youknowriad youknowriad requested review from mtias and jasmussen Mar 3, 2020
@youknowriad youknowriad self-assigned this Mar 3, 2020
@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Mar 3, 2020

Size Change: +751 B (0%)

Total Size: 866 kB

Filename Size Change
build/block-editor/index.js 105 kB +27 B (0%)
build/block-editor/style-rtl.css 10.5 kB -19 B (0%)
build/block-editor/style.css 10.5 kB -18 B (0%)
build/block-library/editor-rtl.css 7.36 kB -41 B (0%)
build/block-library/editor.css 7.36 kB -41 B (0%)
build/block-library/index.js 116 kB -5 B (0%)
build/block-library/style-rtl.css 7.5 kB -3 B (0%)
build/block-library/style.css 7.51 kB -3 B (0%)
build/components/style-rtl.css 15.6 kB +36 B (0%)
build/components/style.css 15.5 kB +38 B (0%)
build/edit-post/index.js 91.5 kB +596 B (0%)
build/edit-post/style-rtl.css 8.65 kB +122 B (1%)
build/edit-post/style.css 8.65 kB +122 B (1%)
build/editor/index.js 44.6 kB -10 B (0%)
build/editor/style-rtl.css 3.98 kB -25 B (0%)
build/editor/style.css 3.98 kB -25 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 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/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 191 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-site/index.js 4.63 kB 0 B
build/edit-site/style-rtl.css 2.51 kB 0 B
build/edit-site/style.css 2.51 kB 0 B
build/edit-widgets/index.js 4.42 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 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.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 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/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 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

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 3, 2020

Nice work.

The color is a bit off, the icon size is a bit off, and we should not use the icon version of the logo as that's been hinted to be shown small. Mind if I fix all of this and push straight to the branch?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Mar 3, 2020

@jasmussen go for it.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 3, 2020

I'm trying to include a new logo SVG but getting some linting errors:

Screenshot 2020-03-03 at 10 32 18

✖ wp-scripts lint-js found some errors. Please fix them and try committing again.

/Users/joen/GIT/gutenberg/packages/edit-post/src/components/header/fullscreen-mode-close/index.js
13:1  error  '@wordpress/primitives' should be listed in the project's dependencies. Run 'npm i -S @wordpress/primitives' to add it  import/no-extraneous-dependencies

✖ 1 problem (1 error, 0 warnings)

What am I missing?

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 3, 2020

Nevermind, figured it out. Going to push a fix to the hover color and focus.

In the mean time — the tooltip says "View Posts" — can you help me change that to "Go to Posts"?

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 3, 2020

Pushed some polish:

button

As mentioned, the label should say "Go to Posts" rather than "View Posts".

The alternative design proposed in #20579 requires a bit of a different tooltip. We may want to not close that ticket with this PR, but keep it open, and then try this tooltip for starters.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Mar 3, 2020

In the mean time — the tooltip says "View Posts" — can you help me change that to "Go to Posts"?

As explained in the PR description that's not possible because of the CPTs.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 3, 2020

Oh, apologies, I missed that! As you were, then :)

youknowriad added 2 commits Mar 3, 2020
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Mar 5, 2020

This works as expected for me. I did notice some potential variation in drop down we might want to correct:

image

This, however, I think is another issue. cc'ing in @jasmussen as might be due to recent updates?

For now, removing design feedback label.

@karmatosed karmatosed self-requested a review Mar 5, 2020
Copy link
Member

karmatosed left a comment

Approving based on design.

@youknowriad youknowriad merged commit cf7f028 into master Mar 5, 2020
3 checks passed
3 checks passed
build
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
WordPress 5.4 Must Have automation moved this from Needs Review to Done Mar 5, 2020
@youknowriad youknowriad deleted the update/fullscreen-wp-button branch Mar 5, 2020
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Mar 5, 2020

Let's try that.

@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Mar 9, 2020

I feel it is too late to add a new default full screen setup into WP 5.4. We should instead have time to test it out in the plugin. Lets punt it to WP 5.5.

@irishetcher

This comment has been minimized.

Copy link

irishetcher commented Mar 11, 2020

I like the fullscreen mode but probably won't use it. It will for be become forgotten very quickly due to the fact that the control is hidden in the drop down control. If there was a discrete control up front on the tool bar I would be more inclined to use it by default.

@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Mar 11, 2020

Hey @irishetcher

I made this issue: #20695
Where I am suggesting to have a full screen icon always available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.