#7558: Improve document/form builder responsiveness #7976
Conversation
fregante
force-pushed
the
F/mv3/page-editor-wrapping
branch
from
cd68a6d
to
0a8d979
Compare
fregante
force-pushed
the
F/mv3/page-editor-wrapping
branch
from
0a8d979
to
98fc81a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7976 +/- ##
==========================================
+ Coverage 72.98% 73.00% +0.01%
==========================================
Files 1307 1309 +2
Lines 40588 40593 +5
Branches 7538 7531 -7
==========================================
+ Hits 29625 29635 +10
+ Misses 10963 10958 -5 ☔ View full report in Codecov by Sentry. |
fregante
changed the title
ActiveField and FormIntroFields components
| @@ -34,6 +35,7 @@ const FormBuilder: React.FC<{ | |||
| return ( | |||
| <div className={styles.root}> | |||
| <div className={styles.column} data-testid="editor"> | |||
| <FormIntroFields name={name} /> | |||
There was a problem hiding this comment.
These fields are only used once or twice, I extracted this component from FormEditor
| @@ -0,0 +1,86 @@ | |||
| import React from "react"; | |||
| import { | |||
There was a problem hiding this comment.
These two components can't be made strict, see automated comment:
#7976 (comment)
I think I made no changes to ActiveField other than extracting it.
| className={styles.header} | ||
| onClick={onToggle} | ||
| ref={headerRef} | ||
| onKeyPress={onToggle} |
There was a problem hiding this comment.
a11y: we should probably forbid role="button" for the most part, and instead use button. button also comes with "onKeyPress" natively
There was a problem hiding this comment.
From the screenshot, this PR appears to be removing the "Use the preview tab" signifier/instructional text |
Restored, screenshot updated. This PR is now ready for review. strictNullChecks CI: the files can't be added |
fregante
changed the title
There was a problem hiding this comment.
Approving -- wondering if it's possible to lump a style change on the "Use preview tab" text so that they match between the two? @fregante
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
|
@BrandonPxBx thank you!
Gotta check how similar they are under the hood, but there only two differences on the surface:
Edit: I checked and the components would require a bit too many changes right now, they are wrapped around different components |
| </Row> | ||
| </p> | ||
|
|
||
| <h6>Current element: {currentElementName}</h6> |
There was a problem hiding this comment.
NIT: not a fan of using an h6. <p><strong>...</strong><p> might make more sense here?
In general, we shouldn't be skipping heading types. If we want it to be a heading, then we should use the appropriate level and re-style.
Very far from a priority.
There was a problem hiding this comment.
I think skipping heading levels is not an issue, especially since we don't really use them consistently. However since it's a section header, any heading tag is still preferable to p>strong IMO
What does this PR do?
complexitylint rule, as suggested inESLint: jsx-max-depth #7819 (comment)
Form Editor
Element Editor
I tried to somewhat match the existing style of the form editor here as well
Checklist