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

Text Style: Reset font weight to regular on font family change #5360

Merged
merged 6 commits into from
Dec 5, 2020

Conversation

zachhale
Copy link
Contributor

@zachhale zachhale commented Nov 19, 2020

Summary

Bring back fix for #1899 and fix lingering bug. The original fix (#4900) implemented an almost complete fix, but left one bug which reverted any content changes in the currently editing text element when the font family was changed.

This PR re-applies the original fix and resolves the bug with content getting reverted when family changes.

Original fix PR: #4900
Revert PR: #5045

Relevant Technical Choices

This was a tricky one because the editor needs to get destroyed so that we can (1) set the font weight to the whole element and (2) make sure any content changes are saved first.

There's no way to have the appropriate content to clean up until after the editor closes, so this PR introduces a "queued push" concept that waits for selectedElements to update before resetting the font weight on the element.

To-do

User-facing changes

Testing Instructions

  1. Add text element
  2. Type Hey. Hit enter then type more text again
  3. Change the font weight on part of the text
  4. Highlight any part of the text or ensure the cursor is in the editor
  5. Go to font picker and select any font family
  6. Editor should go away, font family should change, and all font weights should reset to be one single font weight

Fixes #1899

@google-cla google-cla bot added the cla: yes label Nov 19, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 19, 2020

Size Change: +560 B (0%)

Total Size: 1.5 MB

Filename Size Change
assets/js/edit-story.js 574 kB +243 B (0%)
assets/js/stories-dashboard.js 639 kB +317 B (0%)
ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 876 B 0 B
assets/css/stories-dashboard.css 929 B 0 B
assets/css/web-stories-embed-block.css 550 B 0 B
assets/js/chunk-fonts-********************.js 44.1 kB 0 B
assets/js/chunk-web-stories-template-0-********************.js 10.7 kB 0 B
assets/js/chunk-web-stories-template-1-********************.js 11.2 kB 0 B
assets/js/chunk-web-stories-template-2-********************.js 11 kB 0 B
assets/js/chunk-web-stories-template-3-********************.js 11.2 kB 0 B
assets/js/chunk-web-stories-template-4-********************.js 12.6 kB 0 B
assets/js/chunk-web-stories-template-5-********************.js 7.07 kB 0 B
assets/js/chunk-web-stories-template-6-********************.js 10.2 kB 0 B
assets/js/chunk-web-stories-template-7-********************.js 10.3 kB 0 B
assets/js/chunk-web-stories-textset-0-********************.js 5.25 kB 0 B
assets/js/chunk-web-stories-textset-1-********************.js 6.54 kB 0 B
assets/js/chunk-web-stories-textset-2-********************.js 7.83 kB 0 B
assets/js/chunk-web-stories-textset-3-********************.js 15.2 kB 0 B
assets/js/chunk-web-stories-textset-4-********************.js 4.38 kB 0 B
assets/js/chunk-web-stories-textset-5-********************.js 5.68 kB 0 B
assets/js/chunk-web-stories-textset-6-********************.js 5.47 kB 0 B
assets/js/chunk-web-stories-textset-7-********************.js 10.3 kB 0 B
assets/js/web-stories-activation-notice.js 74 kB 0 B
assets/js/web-stories-embed-block.js 17.4 kB 0 B

compressed-size-action

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #5360 (227f3e6) into main (4af6d49) will decrease coverage by 0.01%.
The diff coverage is 79.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5360      +/-   ##
==========================================
- Coverage   84.48%   84.47%   -0.02%     
==========================================
  Files        1002     1003       +1     
  Lines       17647    17685      +38     
==========================================
+ Hits        14909    14939      +30     
- Misses       2738     2746       +8     
Flag Coverage Δ
karmatests 73.10% <72.41%> (-0.02%) ⬇️
unittests 65.86% <79.31%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...src/edit-story/components/panels/textStyle/font.js 100.00% <ø> (ø)
...mponents/panels/textStyle/useRichTextFormatting.js 88.67% <61.53%> (-8.89%) ⬇️
...omponents/panels/textStyle/getClosestFontWeight.js 92.85% <92.85%> (ø)
...it-story/components/panels/textStyle/fontPicker.js 100.00% <100.00%> (ø)
assets/src/edit-story/app/api/apiProvider.js 52.83% <0.00%> (-1.17%) ⬇️
includes/Story_Post_Type.php 91.20% <0.00%> (+0.07%) ⬆️
assets/src/edit-story/app/story/storyProvider.js 96.87% <0.00%> (+0.10%) ⬆️
...s/src/edit-story/app/story/effects/useLoadStory.js 92.59% <0.00%> (+0.28%) ⬆️
includes/REST_API/Stories_Controller.php 57.36% <0.00%> (+1.01%) ⬆️

@zachhale zachhale marked this pull request as ready for review December 3, 2020 06:50
Copy link
Contributor

@BrittanyIRL BrittanyIRL left a comment

Choose a reason for hiding this comment

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

This makes sense! Well done.

I think added a karma test for this edge case would be useful. It works great!

Comment on lines 106 to 108
pushUpdate(({ content }) => {
return { content: updater(content, ...args) };
}, true),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this doesn't need the brackets, can just keep it how it was I think?

);

// when selected elements update, run any queued pushes
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 👏 👏

@zachhale
Copy link
Contributor Author

zachhale commented Dec 3, 2020

OPE, looks like those tests were moved to a different file while I was working on this. Brought them back!

@zachhale zachhale merged commit d172b35 into main Dec 5, 2020
@zachhale zachhale deleted the bug/1899-reset-font-weight branch December 5, 2020 21:59
@swissspidy swissspidy added Elements: Text Group: Style Panel Type: Bug Something isn't working Type: Enhancement New feature or improvement of an existing feature labels Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Elements: Text Group: Style Panel Type: Bug Something isn't working Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing font family should result in 400 for font weight
4 participants