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

Allows edit text as soon as user type while selecting a text element #1810

Merged
merged 8 commits into from
Jun 12, 2020

Conversation

obetomuniz
Copy link
Contributor

@obetomuniz obetomuniz commented May 15, 2020

Summary

This PR fixes text element edit mode to allows insert content when user select text element and press some character

Relevant Technical Choices

After some debug work, I noticed that we have some similar expected behavior on the already working implementation of Enter action, so I just improve the method to allow both scenarios.


Solving #830
Fixes #1608

@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

Merging #1810 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1810      +/-   ##
============================================
- Coverage     65.93%   65.92%   -0.01%     
  Complexity      304      304              
============================================
  Files           635      635              
  Lines         10342    10340       -2     
============================================
- Hits           6819     6817       -2     
  Misses         3523     3523              
Impacted Files Coverage Δ Complexity Δ
assets/src/edit-story/elements/text/frame.js 93.18% <100.00%> (-0.30%) 0.00 <0.00> (ø)

@github-actions
Copy link
Contributor

github-actions bot commented May 15, 2020

Size Change: -16 B (0%)

Total Size: 802 kB

Filename Size Change
assets/js/edit-story.js 366 kB -10 B (0%)
assets/js/stories-dashboard.js 419 kB -6 B (0%)
ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 269 B 0 B
assets/css/stories-dashboard.css 305 B 0 B
assets/css/web-stories-embed-block.css 501 B 0 B
assets/js/web-stories-embed-block.js 15.7 kB 0 B

compressed-size-action

@dvoytenko dvoytenko removed their request for review May 18, 2020 01:02
* master: (77 commits)
  Fix issue templates headers
  Bump karma from 5.0.5 to 5.0.8 (#1851)
  add allow-empty-input in linter so that changes in storybook js don't fail in pre-commit (#1850)
  add FlagsProvider to storybook
  simplify jasmine eslint rules
  Bump @testing-library/dom from 7.5.6 to 7.5.7 (#1829)
  Bump react-moveable from 0.20.6 to 0.20.7 (#1827)
  Bump puppeteer from 3.0.4 to 3.1.0 (#1826)
  review fixes
  Karma: puppeeter support and native events
  ignore karma tests from coverage
  review fixes
  Update karma.config.js
  Update .eslintrc
  Incrase timeout for visual regression test (#1785)
  Dashboard: Adds Tooltips. Borders around Preview Cards. (#1800)
  Update issue & PR templates (#1824)
  Add docs about Stories Labs env (#1815)
  Bump @wordpress/e2e-test-utils from 4.6.0 to 4.7.0 (#1790)
  Add a basic database upgrader. (#1751)
  ...
Copy link
Contributor

@barklund barklund left a comment

Choose a reason for hiding this comment

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

This works great. And this also fixes #1608 btw!

* master: (101 commits)
  Update list of Google Fonts
  Fix google fonts build script
  Use data arg for force=true instead of as a literal string
  Added call to delete API for media in apiProvider
  Fix using incorrect input for adding color preset. (#1933)
  only use shapes and background colors for dummy story data to avoid local host image issues
  Bump percy from 0.26.6 to 0.26.7 (#1937)
  Renamed animation components for better clarity
  Add custom calendar implementation (#1743)
  Fixed lint errors
  Added tests to AMPKeyframes
  Bump uuid from 8.0.0 to 8.1.0 (#1909)
  Bump @wordpress/icons from 1.4.0 to 2.0.0 (#1928)
  Bring back eslint-plugin-react-hooks and fix issues (#1589)
  Use use-context-selector versions of createContext, useContext
  Add useContextSelectorShallow hook
  Moved SimpleAnimation to root of parts
  Dashboard: Converted BlinkOn animation to new standard
  splitting out storiesView and emptyView from content. Adding tests in content/test and storybook examples in content/ .
  Force stylelint to 13.3.3, because 13.5.0 was failing
  ...
Copy link
Contributor

@barklund barklund left a comment

Choose a reason for hiding this comment

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

Now that we have the new integration test setup, could we add a test for this too? Both entering edit mode by clicking, pressing enter and typing a key? And of course validate selection and text field contents afterwards?

* master: (133 commits)
  Update build script
  Update release assets workflow
  Bump @wordpress/data from 4.18.0 to 4.19.0 (#2114)
  Bump @wordpress/i18n from 3.12.0 to 3.13.0 (#2110)
  Bump @wordpress/e2e-test-utils from 4.7.0 to 4.8.0 (#2116)
  Bump @storybook/addon-actions from 6.0.0-beta.15 to 6.0.0-beta.17 (#2118)
  Bump lint-staged from 10.2.6 to 10.2.7 (#2113)
  Update eslint-plugin-react-hooks
  Unlock stylelint version (#1913)
  Bump @storybook/client-api from 6.0.0-beta.15 to 6.0.0-beta.17 (#2063)
  Bump @testing-library/jest-dom from 5.8.0 to 5.9.0 (#2111)
  Bump @storybook/addon-viewport from 6.0.0-beta.15 to 6.0.0-beta.17 (#2064)
  Bump @storybook/addon-knobs from 6.0.0-beta.15 to 6.0.0-beta.17 (#2065)
  Bump @storybook/addon-a11y from 6.0.0-beta.15 to 6.0.0-beta.17 (#2062)
  Bump @storybook/addon-backgrounds from 6.0.0-beta.15 to 6.0.0-beta.17 (#2061)
  Bump @storybook/addon-storysource from 6.0.0-beta.15 to 6.0.0-beta.17 (#2066)
  Bump karma-jasmine from 3.2.0 to 3.3.1 (#2102)
  Bump @storybook/react from 6.0.0-beta.15 to 6.0.0-beta.17 (#2068)
  Add storybook story for failed preview dialog (#2080)
  Bump @testing-library/dom from 7.5.9 to 7.6.0 (#2103)
  ...
* master:
  Karma: improve and document the debug mode
  Fix eslint on master
  reset added & tests updated
  update keyframes to supported format
  polyfill working on safari storybook
  wip, making sure amp anims not broken on safari
  WIP animations working on safari
  polyfill web animations api
  added tests & updated story to utilize new methods
  unifying aggregate anim methods
@obetomuniz obetomuniz requested a review from barklund May 29, 2020 19:55
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 1, 2020
@barklund
Copy link
Contributor

barklund commented Jun 1, 2020

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jun 1, 2020
Copy link
Contributor

@barklund barklund left a comment

Choose a reason for hiding this comment

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

Tests are good, progressing to QA

@merapi
Copy link
Contributor

merapi commented Jun 2, 2020

@obetomuniz I found 3 issues (2 in gif), should we handle them here?

  • Pressing Enter or typing text doesn't work as expected for a newly inserted text element (in tests we click on the canvas first, so maybe it's related to focus issues?).
  • After editing the text and then exiting (Escape) - Enter or typing doesn't work as expected.
  • ^Same when the text is lasso selected.

texttype

@obetomuniz
Copy link
Contributor Author

obetomuniz commented Jun 3, 2020

Hey @merapi, I already mapped them for a follow-up PR on #1808 and #1830 since it is not happening only with Text Element.

@merapi
Copy link
Contributor

merapi commented Jun 4, 2020

Hey @merapi, I already mapped them for a follow-up PR on #1808 and #1830 since it is not happening only with Text Element.

Ok, I guess it's the same case with lasso selection.

@barklund
Copy link
Contributor

barklund commented Jun 8, 2020

@obetomuniz, are you following up on this after QA failed?

@obetomuniz
Copy link
Contributor Author

obetomuniz commented Jun 8, 2020

Hey @barklund, the QA failing on #830 is related to PR#2183, not this one, or should I consider something else from that report? Likewise, the QA reported on #1608 is related to #1808, which will be fixed by the global focus issue PR. Should I block this one until we get global focus issue solved?

@barklund barklund merged commit 49db6eb into master Jun 12, 2020
@barklund barklund deleted the fix/keydown-on-text-element branch June 12, 2020 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Elements: Text Type: Bug Something isn't working
Projects
None yet
4 participants