Consecutive text placement #4218

miina · 2020-08-25T12:56:20Z

Summary

Adds logic for consecutively placing text presets.

Relevant Technical Choices

Positions elements under each other when added consecutively.
If there is any change made between adding the presets, the position will be reset, too.

To-do

Tests

User-facing changes

Text presets are positioned if clicked on consecutively:

Testing Instructions

I

Add different text presets consecutively.
Verify that if presets are added right after each other, the next preset is positioned below the previous.
Verify that the preset doesn't go out of the bottom of the page, staggering starts from the default position (vertically centered) if the room ends up in the bottom of the page.
Verify that if any change is made between adding the presets, the preset is added to the default position (the consecutiveness counts only if the presets are added right after each other).

Fixes #1924

github-actions · 2020-08-25T13:01:30Z

Size Change: +1.38 kB (0%)

Total Size: 1.16 MB

Filename	Size	Change
`assets/js/edit-story.js`	489 kB	+890 B (0%)
`assets/js/stories-dashboard.js`	514 kB	+494 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	268 B	0 B
`assets/css/stories-dashboard.css`	305 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.3 kB	0 B
`assets/js/chunk-web-stories-template-2-********************.js`	10 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.86 kB	0 B
`assets/js/chunk-web-stories-template-6-********************.js`	9.91 kB	0 B
`assets/js/chunk-web-stories-template-7-********************.js`	9.78 kB	0 B
`assets/js/web-stories-activation-notice.js`	63.6 kB	0 B
`assets/js/web-stories-embed-block.js`	16.9 kB	0 B

_{compressed-size-action}

codecov · 2020-08-25T13:08:06Z

Codecov Report

Merging #4218 into main will increase coverage by 2.61%.
The diff coverage is 86.44%.

@@            Coverage Diff             @@
##             main    #4218      +/-   ##
==========================================
+ Coverage   80.25%   82.87%   +2.61%     
==========================================
  Files         824      830       +6     
  Lines       14334    14482     +148     
==========================================
+ Hits        11504    12002     +498     
+ Misses       2830     2480     -350

Flag	Coverage Δ
#karmatests	`70.78% <83.05%> (-0.32%)`	⬇️
#unittests	`66.16% <57.62%> (+0.52%)`	⬆️

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

Impacted Files	Coverage Δ
...sets/src/edit-story/app/history/historyProvider.js	`88.88% <ø> (ø)`
...story/components/library/panes/text/textPresets.js	`100.00% <ø> (ø)`
...ets/src/edit-story/utils/getInsertedElementSize.js	`66.66% <66.66%> (ø)`
...c/edit-story/components/canvas/useInsertElement.js	`97.56% <100.00%> (+14.51%)`	⬆️
...it-story/components/library/panes/text/textPane.js	`100.00% <100.00%> (+14.28%)`	⬆️
...y/components/library/panes/text/useInsertPreset.js	`100.00% <100.00%> (ø)`
assets/src/edit-story/units/dimensions.js	`100.00% <100.00%> (ø)`
...ssets/src/edit-story/utils/useElementsWithLinks.js	`78.57% <0.00%> (-9.90%)`	⬇️
...edit-story/components/panels/videoAccessibility.js	`94.44% <0.00%> (-5.56%)`	⬇️
assets/src/edit-story/elements/video/controls.js	`88.23% <0.00%> (ø)`
... and 69 more

barklund

A few comments.

I don't really feel this makes the editor experience better, but if this is what UX thinks is best, I'm all for it.

Maybe I just don't understand why we don't simply stagger all texts added regardless of hierarchy.

assets/src/edit-story/components/library/panes/text/karma/textPane.karma.js

assets/src/edit-story/components/library/panes/text/textPane.js

barklund

This works as expected. Just a few minor comments.

assets/src/edit-story/components/library/panes/text/textPane.js

barklund · 2020-08-26T23:10:15Z

assets/src/edit-story/components/library/panes/text/textPane.js

+      if (
+        !lastPreset.current ||
+        versionNumber - lastPreset.current.versionNumber !== 1
+      ) {


It's a bit tricky to rely on version number only accumulating once. For instance, auto-save will probably update history, which might conflict with this. The "good" solution would be to only set lastPreset after the history has updated and then clear it if history updates again in-between. But with the current structure that's not really possible. We don't have a way to say "and after the next update, do this".

Hmm, auto-save/save shouldn't really update the version number/history though -- it should only update the saved version number. If auto-save (or save) does that then there's an issue with history, perhaps related to #4094

versionNumber - lastPreset.current.versionNumber !== 1 is actually the check to see if the history has updated meanwhile -- in any other case then the difference being 1 something other than adding a preset has happened.

Just tested that currently saving the story seems to modify the history indeed (e.g. saving and then clicking Undo does nothing, indicating that the save itself has modified something) -- this would need investigation, will add a note to #4094.

Changed the logic a bit in 1704efb -- let me know if it's a bit more clear now. However, note that due to the history bug it still stops the consecutive ordering when saving/auto-saving. 🤷
I think we can live with that until the history bug gets fixed though. Debugged it a bit and saving, or loading a new story indeed seems to create a new (identical to the previous) history entry into history -- perhaps there's object mutation happening somewhere instead of returning the original object.

Thoughts?

I think the new structure works well.

The "bug" is due to us post-save updating the story with the correct link and status of the story - e.g. if the user has changed permalink settings since last saving, the permalink will come back changed when saving is complete.

The bug could then be considered that history is reported as changed, even though no attribute actually updated. But saving can and should be able to update the story and thus progress history, due to the above possible changes happening outside the editor.

assets/src/edit-story/components/library/panes/text/karma/textPane.karma.js

assets/src/edit-story/components/library/panes/text/useInsertPreset.js

barklund · 2020-08-27T21:10:40Z

assets/src/edit-story/components/library/panes/text/karma/textPane.karma.js

+    await fixture.events.click(fixture.editor.library.text.preset('Paragraph'));
+    await fixture.events.click(fixture.editor.library.text.preset('OVERLINE'));


Hm, there's something fishy in the screenshot though. Overline is added to the bottom, while the paragraph wrapped up at the original starting point. Overline added last should have gone right below the last paragraph (added at the original offset).

I think the history didn't manage to update between these, which is why this can occur. It's not super critical but might make the test a bit flaky.

Maybe you should create a wrapper method that makes sure to wait for history to update before proceeding with the next text insertion.

WDYT?

Hmm, looks like the issue is with the first "Heading 1" and the first "Paragraph" -- it looks like the first paragraph was added on top of Heading 1 instead of below it -- the following elements seem to be staggered correctly after that.

Indeed it looks like the history didn't update properly meanwhile -- seems to be an issue with the test only.

miina · 2020-08-31T13:34:56Z

Will follow up on making the tests more robust in a separate PR.

Improves flaky tests from #4218 (text staggering)

miina added 2 commits August 25, 2020 14:40

Start adding rules for adding consequtive text elements.

c8691cb

Consider changes between adding presets

aa669a1

google-cla bot added the cla: yes label Aug 25, 2020

Add test for adding text presets.

eaeedd7

miina marked this pull request as ready for review August 25, 2020 18:57

barklund assigned miina Aug 25, 2020

barklund added the Pod: Prometheus (Workspace) label Aug 25, 2020

barklund reviewed Aug 25, 2020

View reviewed changes

miina marked this pull request as draft August 25, 2020 20:40

miina added 6 commits August 26, 2020 11:51

Merge remote-tracking branch 'origin/main' into add/1924-text-placement

78a5d80

Stagger all text presets.

0f1552a

Move code around a little bit.

c9241ce

Add logic for staggering.

984a8cd

Adjust tests.

0b6805a

Fix tests.

acf4978

miina marked this pull request as ready for review August 26, 2020 10:36

miina requested review from merapi and barklund August 26, 2020 10:36

Another test fix try.

611adad

barklund approved these changes Aug 26, 2020

View reviewed changes

barklund reviewed Aug 26, 2020

View reviewed changes

assets/src/edit-story/components/library/panes/text/karma/textPane.karma.js Outdated Show resolved Hide resolved

miina added 3 commits August 27, 2020 11:22

Improve test.

bd6f3b6

Add useInsertPreset helper.

176abea

Try to make the logic of detecting history changes a bit more clear.

1704efb

barklund reviewed Aug 27, 2020

View reviewed changes

assets/src/edit-story/components/library/panes/text/useInsertPreset.js Show resolved Hide resolved

barklund reviewed Aug 27, 2020

View reviewed changes

miina added 2 commits August 28, 2020 15:27

Adjust tests a bit.

139bb25

Revert changes.

e38d192

miina merged commit 177aa10 into main Aug 31, 2020

miina deleted the add/1924-text-placement branch August 31, 2020 13:35

swissspidy added Elements Elements: Text Type: Enhancement New feature or improvement of an existing feature labels Aug 31, 2020

miina mentioned this pull request Sep 3, 2020

History: Opening an existing story indicates new changes right away #4094

Closed

miina added a commit that referenced this pull request Sep 3, 2020

Merge pull request #4327 from google/fix/4218-flaky-karma

8fb9af9

Improves flaky tests from #4218 (text staggering)

swissspidy mentioned this pull request Apr 27, 2023

Remove unused props from default templates and save requests #13221

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consecutive text placement #4218

Consecutive text placement #4218

miina commented Aug 25, 2020 •

edited

Loading

github-actions bot commented Aug 25, 2020 •

edited

Loading

codecov bot commented Aug 25, 2020 •

edited

Loading

barklund left a comment

barklund left a comment

barklund Aug 26, 2020

miina Aug 27, 2020 •

edited

Loading

miina Aug 27, 2020

barklund Aug 27, 2020

barklund Aug 27, 2020

miina Aug 28, 2020

miina commented Aug 31, 2020

		await fixture.events.click(fixture.editor.library.text.preset('Paragraph'));
		await fixture.events.click(fixture.editor.library.text.preset('OVERLINE'));

Consecutive text placement #4218

Consecutive text placement #4218

Conversation

miina commented Aug 25, 2020 • edited Loading

Summary

Relevant Technical Choices

To-do

User-facing changes

Testing Instructions

github-actions bot commented Aug 25, 2020 • edited Loading

codecov bot commented Aug 25, 2020 • edited Loading

Codecov Report

barklund left a comment

Choose a reason for hiding this comment

barklund left a comment

Choose a reason for hiding this comment

barklund Aug 26, 2020

Choose a reason for hiding this comment

miina Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

miina Aug 27, 2020

Choose a reason for hiding this comment

barklund Aug 27, 2020

Choose a reason for hiding this comment

barklund Aug 27, 2020

Choose a reason for hiding this comment

miina Aug 28, 2020

Choose a reason for hiding this comment

miina commented Aug 31, 2020

miina commented Aug 25, 2020 •

edited

Loading

github-actions bot commented Aug 25, 2020 •

edited

Loading

codecov bot commented Aug 25, 2020 •

edited

Loading

miina Aug 27, 2020 •

edited

Loading