fix/115: overflow charmap popver #130
Conversation
Sidsector9
requested review from
a team and
peterwilsoncc
and removed request for
a team
src/index.js
Outdated
| @@ -46,7 +46,42 @@ registerFormatType( type, { | |||
| }; | |||
| // Pin the Popover to the caret position. | |||
| const anchorRect = () => { | |||
| return getRectangleFromRange( anchorRange ); | |||
| /** | |||
There was a problem hiding this comment.
| /** | |
| /* |
Nitpick: Only docblocks use **, multi-line comments use a single asterisks.
There was a problem hiding this comment.
@peterwilsoncc I made this change. Out of curiosity can you also explain on the rationale behind this?
There was a problem hiding this comment.
can you also explain on the rationale behind this?
The short version is that it meets the WP coding standards but I can take an educated guess as to the reason behind it.
The phpdoc spec defines a docblock as starting with a double asterisks. The JSDoc spec uses the same convention.
My guess is that it helps with parsing the code files in to something more useful, such as the developer.wordpress.org pages. If a comment begins with /** then the parser knows it needs to check whether the comment is a valid docblock.
If a comment only begins with /* then the parser knows it can be skipped and considered an inline comment.
|
@peterwilsoncc I'm unable to reproduce the issue on Firefox 101. Are you testing on Firefox 102 beta? |
There was a problem hiding this comment.
Are you testing on Firefox 102 beta?
I was but I figured out the issue is something different.
I'd left Gutenberg 13.3.0 active and had thought I'd disabled it. The issue is not apparent in the latest version of the plugin, 13.4.0.
Placement is as expected in the following configs so I'll approve.
- WordPress 6.0
- WordPress 5.9
- Both of above with Gutenberg 13.4.0
There was a problem hiding this comment.
I've noticed 8b91edb includes a large change to package-lock.json
Was that intentional?
|
@peterwilsoncc I was confused myself as running |
|
In prepping for a new release I was looking through why our e2e tests were failing (especially after I merged in a PR from dependabot that broke them further). We have some timeout issues we're running into but there's also an error that was introduced from this PR in our minimum environment only. In those tests, we're getting: Looking at the code here, I'm assuming it's because the I think it may be as simple as checking if that element exists and if not, just return the value we were using previously. Otherwise we can run these new calculations. Curious on other's thoughts though, /cc @Sidsector9 @jeffpaul @peterwilsoncc |
|
@dkotter I'd also be fine bumping the WP minimum to something more current, would be curious if we're able to determine when that element exists and if setting that as a minimum would work for us. |
|
It looks like the classes were renamed around 5.4, see WordPress/gutenberg@de7c442 |
|
In that case, I'm totally fine bumping the WP minimum to 5.4 (or frankly even up to 6.0 if that made things easier to maintain here). |
|
5.4 is our current minimum. In running some tests, seems the HTML changes were made in 5.5, so I've opened a PR that bumps our minimum to 5.5 |
Description of the Change
Computes the
Rect.xposition depending on the editor viewport and the range selection rectangle widths.The user experience is a bit glitchy especially when the overlap happens on the left border, however the feature is 100% usable. Once this is fixed in Gutenberg Core, we can revert this fix.
Closes #115
Screenshot
Before (left)
Before (right)
After (left)
(right)
Verification Process
Test the fix on the following themes by comparing with the screenshots above:
Checklist:
Changelog Entry
Fixed: inserter popup overflow which prevents user from selecting few characters.
Credits
Props @cldhmmr @Sidsector9