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

Expand the multi-entity saving panel by default #27437

Merged
merged 3 commits into from
Dec 2, 2020

Conversation

youknowriad
Copy link
Contributor

Follow up to #26355

Based on some discussion with @jameskoster and @jasmussen it was decided that this multi-entity saving panel should be expanded by default.

Testing instructions

  • Enable an FSE theme
  • Add a site title block to a post
  • Edit the site title
  • Try to publish your post

@youknowriad youknowriad added Needs Design Feedback Needs general design feedback. [Feature] Full Site Editing [Type] Experimental Experimental feature or API. labels Dec 2, 2020
@youknowriad youknowriad self-assigned this Dec 2, 2020
Copy link
Contributor

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

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

Working as advertised.

Screenshot 2020-12-02 at 10 30 10

@youknowriad
Copy link
Contributor Author

I just moved ahead and removed the ability to expand the changes entirely. What do you think?

@jameskoster
Copy link
Contributor

I think that is a little more tricky. If there are lots of changes in different contexts (template + parts + global styles + content + site data) it may be more convenient to work through them one section at a time, and collapsing may help with that?

No strong opinion here though. Probably fine to remove the collapsibility for now if it's easy to add back. I'll defer to @jasmussen on this :D

@github-actions
Copy link

github-actions bot commented Dec 2, 2020

Size Change: -32 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-library/editor-rtl.css 8.88 kB -6 B (0%)
build/block-library/editor.css 8.88 kB -6 B (0%)
build/block-library/index.js 148 kB +18 B (0%)
build/block-library/style-rtl.css 8.31 kB +16 B (0%)
build/block-library/style.css 8.32 kB +16 B (0%)
build/editor/index.js 43.2 kB -70 B (0%)
build/editor/style-rtl.css 3.85 kB -2 B (0%)
build/editor/style.css 3.85 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 128 kB 0 B
build/block-editor/style-rtl.css 11.2 kB 0 B
build/block-editor/style.css 11.2 kB 0 B
build/block-library/theme-rtl.css 789 B 0 B
build/block-library/theme.css 790 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.95 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.98 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.95 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.42 kB 0 B
build/edit-post/style.css 6.4 kB 0 B
build/edit-site/index.js 24.1 kB 0 B
build/edit-site/style-rtl.css 4.06 kB 0 B
build/edit-site/style.css 4.06 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/element/index.js 4.63 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.82 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 2.92 kB 0 B
build/rich-text/index.js 13.4 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 2.84 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@jasmussen
Copy link
Contributor

I think that is a little more tricky. If there are lots of changes in different contexts (template + parts + global styles + content + site data) it may be more convenient to work through them one section at a time, and collapsing may help with that?

On the one hand, I have a semi strong opinion that the entire reason we have a pre-publish panel in the first place is to show exactly this, so if we don't do that, why even have a prepublish dialog. And furthermore, when a panel defaults to open, and doesn't save its state across sessions, it shouldn't need the ability to collapse.

On the other hand, I recall @mtias expressing, at times, the desire to keep multi entity saving simple, because most of the time and when you are in a site editing context, you know what you're doing.

In absence of him chiming in, I'd say remove it. And if he chimes in later and disagrees, one round on me 😅

Screenshot 2020-12-02 at 11 57 01

This double border annoys me, though 😅 — and now I want to eat at Pizza Planet.

@jasmussen
Copy link
Contributor

Oh wait just to be clear, it's this blue "hide changes" link that I mean we should remove:

Screenshot 2020-12-02 at 11 59 40

I don't mind keeping these collapsible:

Screenshot 2020-12-02 at 12 00 03

@youknowriad
Copy link
Contributor Author

I removed the double border already :)

@youknowriad
Copy link
Contributor Author

@jasmussen that's what I did already.

@jasmussen
Copy link
Contributor

Sigh. One round on me, clearly. You're too fast for me Riad.

@mtias
Copy link
Member

mtias commented Dec 2, 2020

Yes, please! :)

@youknowriad youknowriad merged commit 9092f58 into master Dec 2, 2020
@youknowriad youknowriad deleted the update/entities-saved-state branch December 2, 2020 12:56
@github-actions github-actions bot added this to the Gutenberg 9.6 milestone Dec 2, 2020
@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Dec 3, 2020

Sorry for being late to the party.

Based on some discussion with @jameskoster and @jasmussen it was decided that this multi-entity saving panel should be expanded by default.

I am on board with this change. That was the original idea when moving it over from the old modal.

I just moved ahead and removed the ability to expand the changes entirely. What do you think?

If its open by default, I don't think there is a need for the expand/collapse option.

Pinging @MichaelArestad as he might have some thoughts on this. IIRC he is the one that convinced us to go with trying the hidden by default expanding panel approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants