Story Editor: Preset Panel Height & Collapse Persistence #4601

maxyinger · 2020-09-21T19:00:19Z

Summary

Persists height and collapsed state of all panels in story editor. Panels should remain collapse & height between reloads, element selection & switching stories now.

Relevant Technical Choices

Persisted all panels height & collapsed state.

To-do

NA

User-facing changes

collapse/height state changes to all panels should now maintain between any actions in the editor.

Testing Instructions

Go to edit a story.

Click on an item & resize color preset panel. Click off that item so all panels disappear. Click back on the item and see that the color preset panel height is the same.
Repeat these steps checking if collapse state maintains
Repeat these steps but reload you page between checks
See that other than this, design panels still behave as expected with respect to resize/collapse behavior.

Partially fixes #825

github-actions · 2020-09-21T19:05:53Z

Size Change: +3.73 kB (0%)

Total Size: 1.28 MB

Filename	Size	Change
`assets/js/edit-story.js`	504 kB	+397 B (0%)
`assets/js/stories-dashboard.js`	599 kB	+337 B (0%)
`assets/js/web-stories-activation-notice.js`	74 kB	+2.99 kB (4%)

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	909 B	0 B
`assets/css/stories-dashboard.css`	939 B	0 B
`assets/css/web-stories-embed-block.css`	515 B	0 B
`assets/js/chunk-web-stories-template-0-********************.js`	10.2 kB	0 B
`assets/js/chunk-web-stories-template-1-********************.js`	10.2 kB	0 B
`assets/js/chunk-web-stories-template-2-********************.js`	9.97 kB	0 B
`assets/js/chunk-web-stories-template-3-********************.js`	10.5 kB	0 B
`assets/js/chunk-web-stories-template-4-********************.js`	10.6 kB	0 B
`assets/js/chunk-web-stories-template-5-********************.js`	6.83 kB	0 B
`assets/js/chunk-web-stories-template-6-********************.js`	9.87 kB	0 B
`assets/js/chunk-web-stories-template-7-********************.js`	9.75 kB	0 B
`assets/js/chunk-web-stories-textset-0-********************.js`	1.63 kB	0 B
`assets/js/web-stories-embed-block.js`	17.5 kB	0 B

_{compressed-size-action}

assets/src/edit-story/components/panels/panel/panel.js

assets/src/edit-story/components/panels/preset/presetPanel.js

assets/src/edit-story/components/panels/panel/panel.js

carloskelly13 · 2020-09-21T19:30:48Z

@swissspidy Should we allow a partial panel open like this or size it to the closest color swatch? Doesn't have to be part of this PR, but from a UX standpoint this feels odd. At least for the first row.

assets/src/edit-story/components/panels/panel/panel.js

carloskelly13

This works well. Tested both the sizing of the saved presets and then the collapse states of the other design panels too.

barklund

Looks good, just a few minor comments.

assets/src/edit-story/components/panels/panel/panel.js

barklund · 2020-09-21T21:30:15Z

assets/src/edit-story/components/panels/preset/karma/stylePresets.karma.js

@@ -35,6 +35,9 @@ describe('Panel: Style Presets', () => {
  };

  beforeEach(async () => {
+    // Make sure panels are starting at initial state on every test
+    localStorage.clear();


Hm, this should probably go in some global reset. @swissspidy, suggestions?

Yea let me know here. I'm not the most familiar with the Karma tests, but think this was causing the main issue

Yeah probably. karma/fixture/init.js has similar resets already.

Can be done in a follow-up ticket / PR too, FWIW

swissspidy · 2020-09-21T21:47:32Z

@swissspidy Should we allow a partial panel open like this or size it to the closest color swatch? Doesn't have to be part of this PR, but from a UX standpoint this feels odd. At least for the first row.

@carlos-kelly Good catch! Sounds like an odd thing to allow. There's probably some UX definition for this already. I suggest opening an issue and tagging Sam there.

codecov · 2020-09-21T22:13:38Z

Codecov Report

Merging #4601 into main will increase coverage by 13.22%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #4601       +/-   ##
===========================================
+ Coverage   70.10%   83.33%   +13.22%     
===========================================
  Files         881      881               
  Lines       15519    15444       -75     
===========================================
+ Hits        10880    12870     +1990     
+ Misses       4639     2574     -2065

Flag	Coverage Δ
#karmatests	`72.42% <ø> (+42.09%)`	⬆️
#unittests	`66.30% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ts/src/edit-story/components/panels/panel/panel.js	`81.96% <ø> (+12.57%)`	⬆️
...edit-story/components/panels/preset/presetPanel.js	`95.00% <ø> (+5.00%)`	⬆️
assets/src/edit-story/constants.js	`100.00% <ø> (ø)`
assets/src/activation-notice/theme.js	`66.66% <0.00%> (-13.34%)`	⬇️
includes/Demo_Content.php	`80.85% <0.00%> (-3.08%)`	⬇️
...sets/src/activation-notice/app/components/step2.js	`85.71% <0.00%> (-1.79%)`	⬇️
...sets/src/activation-notice/app/components/step3.js	`85.71% <0.00%> (-1.79%)`	⬇️
assets/src/dashboard/app/views/toaster/index.js	`75.00% <0.00%> (-1.48%)`	⬇️
...sets/src/activation-notice/app/components/step1.js	`87.50% <0.00%> (-1.39%)`	⬇️
includes/Experiments.php	`97.50% <0.00%> (-0.99%)`	⬇️
... and 229 more

… in fixture. leaving in beforeEach for now

pbakaus · 2020-09-22T01:50:40Z

OMG amazing. Thank you!

miina · 2020-09-22T06:54:15Z

For considering later:
In case of presets, it might be good to reset the panel height if all the presets are removed meanwhile. Otherwise, a previous (and now emptied) panel setting could leave unexpected room, e.g.

swissspidy · 2020-09-22T08:20:46Z

assets/src/edit-story/components/panels/panel/panel.js

+      );
+      parsed = JSON.parse(stored);
+    } catch (e) {
+      // @TODO Add some error handling.


Hmm setInitialBannerPreviouslyClosed in useTelemetryOptIn should probably also wrap this in a try/catch

cc @dmmulroy

I have no problem updating it, but what error is this catching? It seems like exceptions can be potentially thrown if the user has prevented a site from using Storage APIs, but none of our other localstorage uses are wrapped in try/catch (see just below). Should we mandate that all localstorage access be wrapped, or a utility to check for access prior to using it?

Also, what is the second argument to getItem? AFAIK it only accepts one and it will already return null by default if a key does not exists which is fine to pass to JSON.parse

swissspidy · 2020-09-22T13:55:48Z

@littlemilkstudio Looking at the Percy screenshots the default size for the layer panel seems to have changed.

Left is before, right is after:

See how the background layer item disappears.

It's even more obvious when editing a template:

Can you double check that?

dmmulroy

This works for me!

dmmulroy · 2020-09-22T14:15:49Z

This works for me!

Just saw Pascal's comment about default size of the lay panel, once that's good I'm good with this 👍

maxyinger · 2020-09-22T16:07:04Z

@littlemilkstudio Looking at the Percy screenshots the default size for the layer panel seems to have changed.

Left is before, right is after:

See how the background layer item disappears.

It's even more obvious when editing a template:

Can you double check that?

Ahh yea, I think Percy has the value persisted between tests. Must have a test on a story where there's only 1 layer initially, so these steps follow:

Open story with 1 layer
uses min of layers or 6 which is 1 layer here to compute initial height
height gets set to persist
open new story with 6 layers, initialHeight is ignored and goes with persisted height from earlier

Updated the behavior to now only cache the state values if the user has interacted with the panel. So it shouldn't persist until the user either adjusts height of the panel or collapses it. This implementation certainly comes with it's edge cases, but this is some UX we should fine tune for a later release and just include this simpler behavior for now imo

Added height & collapse persistance to preset panels

11b9695

google-cla bot added the cla: yes label Sep 21, 2020

cant spell for s#@%

7db3881

maxyinger requested review from barklund, swissspidy, BrittanyIRL, carloskelly13, dmmulroy, mariano-formidable and miina September 21, 2020 19:03

swissspidy reviewed Sep 21, 2020

View reviewed changes

assets/src/edit-story/components/panels/panel/panel.js Outdated Show resolved Hide resolved

swissspidy reviewed Sep 21, 2020

View reviewed changes

assets/src/edit-story/components/panels/preset/presetPanel.js Outdated Show resolved Hide resolved

swissspidy reviewed Sep 21, 2020

View reviewed changes

assets/src/edit-story/components/panels/panel/panel.js Outdated Show resolved Hide resolved

swissspidy added Group: Style Panel Group: Saved Colors Group: Saved Styles Type: Enhancement New feature or improvement of an existing feature labels Sep 21, 2020

pascal suggestions

ebe0c4c

carloskelly13 reviewed Sep 21, 2020

View reviewed changes

assets/src/edit-story/components/panels/panel/panel.js Outdated Show resolved Hide resolved

just persist all panels

d693de1

carloskelly13 approved these changes Sep 21, 2020

View reviewed changes

Maxwell Yinger added 2 commits September 21, 2020 13:59

cleanup constants

9dae148

cleanup constants moar

23b4aa3

maxyinger requested a review from swissspidy September 21, 2020 20:00

maxyinger mentioned this pull request Sep 21, 2020

UI settings don't persist between reloads #825

Closed

clear localStorage on relevant tests

3169656

barklund reviewed Sep 21, 2020

View reviewed changes

parse in try catch. clear local storage in fixture

5af1c1e

maxyinger requested a review from dvoytenko as a code owner September 21, 2020 22:21

maxyinger requested a review from barklund September 21, 2020 22:21

move localStorage to try. still figuring out where to put clearing LS…

70626c2

… in fixture. leaving in beforeEach for now

swissspidy reviewed Sep 22, 2020

View reviewed changes

dmmulroy approved these changes Sep 22, 2020

View reviewed changes

Maxwell Yinger added 2 commits September 22, 2020 09:48

only persist after manual change

5c00940

getItem doesnt take a second arg

1f587a3

maxyinger merged commit 765c6c8 into main Sep 22, 2020

maxyinger deleted the 825-persist-UI-state branch September 22, 2020 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Story Editor: Preset Panel Height & Collapse Persistence #4601

Story Editor: Preset Panel Height & Collapse Persistence #4601

maxyinger commented Sep 21, 2020 •

edited by barklund

Loading

github-actions bot commented Sep 21, 2020 •

edited

Loading

carloskelly13 commented Sep 21, 2020

carloskelly13 left a comment

barklund left a comment

barklund Sep 21, 2020

maxyinger Sep 21, 2020

swissspidy Sep 21, 2020

swissspidy Sep 22, 2020

swissspidy commented Sep 21, 2020

codecov bot commented Sep 21, 2020 •

edited

Loading

pbakaus commented Sep 22, 2020

miina commented Sep 22, 2020

swissspidy Sep 22, 2020

dmmulroy Sep 22, 2020 •

edited

Loading

swissspidy commented Sep 22, 2020

dmmulroy left a comment

dmmulroy commented Sep 22, 2020

maxyinger commented Sep 22, 2020

Story Editor: Preset Panel Height & Collapse Persistence #4601

Story Editor: Preset Panel Height & Collapse Persistence #4601

Conversation

maxyinger commented Sep 21, 2020 • edited by barklund Loading

Summary

Relevant Technical Choices

To-do

User-facing changes

Testing Instructions

github-actions bot commented Sep 21, 2020 • edited Loading

carloskelly13 commented Sep 21, 2020

carloskelly13 left a comment

Choose a reason for hiding this comment

barklund left a comment

Choose a reason for hiding this comment

barklund Sep 21, 2020

Choose a reason for hiding this comment

maxyinger Sep 21, 2020

Choose a reason for hiding this comment

swissspidy Sep 21, 2020

Choose a reason for hiding this comment

swissspidy Sep 22, 2020

Choose a reason for hiding this comment

swissspidy commented Sep 21, 2020

codecov bot commented Sep 21, 2020 • edited Loading

Codecov Report

pbakaus commented Sep 22, 2020

miina commented Sep 22, 2020

swissspidy Sep 22, 2020

Choose a reason for hiding this comment

dmmulroy Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

swissspidy commented Sep 22, 2020

dmmulroy left a comment

Choose a reason for hiding this comment

dmmulroy commented Sep 22, 2020

maxyinger commented Sep 22, 2020

maxyinger commented Sep 21, 2020 •

edited by barklund

Loading

github-actions bot commented Sep 21, 2020 •

edited

Loading

codecov bot commented Sep 21, 2020 •

edited

Loading

dmmulroy Sep 22, 2020 •

edited

Loading