Add reusable modal component. #5367

aaronfc · 2022-07-19T13:39:47Z

Fixes: 1446-gh-Automattic/sensei-pro

Changes proposed in this Pull Request

Implementation based on the same concept of sensei-modal used in Contact Teacher.

Testing instructions

See steps and video here: 1552-gh-Automattic/sensei-pro

Discussion

Should we try to replace the current Contact Teacher implementation with this new module? This is a component so it might need some re-working of how Contact Teacher works right now.
I am using the @wordpress/icon components – it is working (as you can see in the linked pull request video) but maybe we should avoid that? @renatho ?

- Implementation based on the same concept of sensei-modal used in Contact Teacher. -

codecov · 2022-07-19T13:47:58Z

Codecov Report

Merging #5367 (da7e396) into trunk (15a95ac) will increase coverage by 0.01%.
The diff coverage is 76.64%.

❗ Current head da7e396 differs from pull request most recent head 859a60c. Consider uploading reports for the commit 859a60c to get more accurate results

@@             Coverage Diff              @@
##              trunk    #5367      +/-   ##
============================================
+ Coverage     42.85%   42.86%   +0.01%     
  Complexity     8687     8687              
============================================
  Files           412      412              
  Lines         31040    31053      +13     
  Branches        232      234       +2     
============================================
+ Hits          13301    13311      +10     
- Misses        17568    17569       +1     
- Partials        171      173       +2

Impacted Files	Coverage Δ
includes/blocks/class-sensei-blocks.php	`21.87% <0.00%> (ø)`
includes/class-sensei-groups-landing-page.php	`0.00% <ø> (ø)`
includes/class-sensei-posttypes.php	`11.23% <ø> (ø)`
includes/class-sensei-question.php	`12.10% <0.00%> (ø)`
...ourse-video/class-sensei-course-video-settings.php	`26.53% <0.00%> (ø)`
...t-api/class-sensei-rest-api-lessons-controller.php	`9.83% <0.00%> (ø)`
...ss-sensei-reports-overview-list-table-abstract.php	`69.56% <50.00%> (ø)`
.../blocks/course-outline/module-block/module-edit.js	`68.96% <57.14%> (ø)`
includes/class-sensei-quiz.php	`59.04% <66.66%> (ø)`
...locks/class-sensei-course-outline-lesson-block.php	`82.60% <75.00%> (ø)`
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca096c5...859a60c. Read the comment docs.

alexsanford

Was there any cross-talk between the development of this PR and #5361? Maybe there is a need for completely separate components, but it seems like there is some amount of overlap.

I suppose maybe not, since #5361 is a confirmation dialog meant for the editor and this is a modal meant to be shared between the editor and the frontend. But I think it is at least worth a look to make sure we're not duplicating efforts.

fjorgemota · 2022-07-20T19:21:43Z

Hey there.

No, there was no cross-talk between the development of this PR and #5361, mainly because #5361 just uses a modal instead of reimplementing it.

With this said, I don't see any overlap between this PR and #5361. I see an opportunity for improvement for #5361, though, where we could use this modal instead of the one provided by Gutenberg. However, I think it's only worth doing this if the confirmation dialog was going to be also shown on the frontend, which I don't think it's the case.

renatho

I am using the @wordpress/icon components – it is working (as you can see in the linked pull request video) but maybe we should avoid that? @renatho ?

I think that's fine! I checked it, and it's not enqueued by WP, but added to the bundle, and it's super small.

When you need. to check it, you can just run the npm run build:analyzer and check the dependencies.

And about the WP enqueues, you can see the dist files, like this one for this case: assets/dist/interactive-blocks/interactive-blocks-frontend.asset.php. (Check here if you want more details).

assets/shared/components/modal/index.js

assets/shared/components/modal/style.scss

renatho · 2022-07-21T14:30:48Z

assets/shared/components/modal/style.scss

+		overflow: hidden;
+		z-index: 2000;
+		width: 700px;
+		max-width: 96%;


Maybe it could be fullscreen for mobile. WDYT?

Yeah, maybe? Let's keep an eye on this thread p6rkRX-3VB-p2 and let's confirm with Andrei how it should look like. I am also a little concerned about the "actions" available in the modals in the latest designs.

assets/shared/components/modal/style.scss

- Add jsdoc. - Simplify hidden modal by returning `null` instead of hiding with css class. - Receive close callback because modal won't be ever open from the modal itself. - Fix css for height.

aaronfc

Added some minor changes.

Still pending to decide what the design should look like for mobile and if we need to implement something special for "actions".

fjorgemota

Hey there, nice PR overall!

I found an issue while testing this component with 1572-gh-Automattic/sensei-pro with the Twenty Twenty One theme that I think might be worth taking a look.

Basically, this is how the modal appears to me while on Twenty Twenty One (notice that the background also changes while hovering over the modal vs. hovering on the overlay, besides the weird close button):

modal.mp4

At first, I thought it was Firefox being weird, but the same issue also happens on Chrome.

It's worth mentioning that this issue doesn't occur on the editor, and neither occurs when using the theme Twenty Twenty:

modal-other-theme.mp4

The only thing a little weird about this modal on Twenty Twenty theme is that the close button looks...misaligned? Maybe it's worth checking, too, considering that on your video here 1552-gh-Automattic/sensei-pro that doesn't happen.

Apart from that, I left some small comments about the close callback.

Also, it's worth asking: Should this modal still be open if the user presses ESC? Usually, when the user press ESC, the Gutenberg modal calls onClose and so the modal is closed properly, so that's why I'm asking. :)

assets/shared/components/modal/index.js

aaronfc · 2022-07-26T09:02:48Z

The only thing a little weird about this modal on Twenty Twenty theme is that the close button looks...misaligned? Maybe it's worth checking, too, considering that on your video here 1552-gh-Automattic/sensei-pro that doesn't happen.

I tested with Twenty Twenty after the latest changes in CSS and I don't see the misalignment anymore :)

Also, it's worth asking: Should this modal still be open if the user presses ESC? Usually, when the user press ESC, the Gutenberg modal calls onClose and so the modal is closed properly, so that's why I'm asking. :)

I added support for ESC here: ccb8fe1 :D thanks!

Let me know if you see any other strange thing @fjorgemota

fjorgemota

Nice work! Just a few additional comments. =)

assets/shared/components/modal/index.js

renatho

Thank you for the improvements, Aarón! I did a new round of review, and added some extra suggestions here. 😉

renatho · 2022-07-26T20:08:37Z

assets/shared/components/modal/index.js

+		<div className={ 'sensei-modal' }>
+			<button
+				className="sensei-modal sensei-modal__overlay"
+				aria-label="Close"


We need to add the __ here to make it translatable.

Oops! Now that I noticed it's the overall. I think it's not very good from an accessibility perspective. Maybe we could try to use the same as Gutenberg modal? The __experimentalUseFocusOutside.

Reimplemented modal using your suggestions here: 0c029ec

assets/shared/components/modal/index.js

renatho · 2022-07-26T20:41:07Z

assets/shared/components/modal/index.js

+ * @param {Function} props.onClose  Callback to run when trying to close the modal.
+ * @param {string}   props.title    The title for the modal. Empty by default.
+ * @param {Object}   props.children The content of the modal.
+ * @return {JSX.Element} The modal component or null (if not open).


I think we don't need the return here. At some point, we had decided to not add @return to the React components. But if think it's especially important here, feel free to keep it. 😉

Removed in here: 1d4574a

assets/shared/components/modal/index.js

renatho · 2022-07-26T20:49:06Z

assets/shared/components/modal/index.js

+ * @param {Object}   props.children The content of the modal.
+ * @return {JSX.Element} The modal component or null (if not open).
+ */
+const Modal = ( { onClose, title = '', children } ) => {


Another thing that I realized while checking the Gutenberg Modal is that we're not using Portals here. I think it's important for modal implementations, in order to make it work in any place (if it's in a position relative, in a overflow hidden, or any other weird context).

See it here: https://github.com/WordPress/gutenberg/blob/v13.7.3/packages/components/src/modal/index.js#L124

Added portals in here too: 0c029ec

Co-authored-by: Fernando Jorge Mota <fjorgemota@users.noreply.github.com>

- Reimplemented following a similar approach to the Gutenberg original Modal component. - Modal is focused when component is loaded so that keypress events are handled throw the `onKeyDown` attribute. - Modal is closed when unfocused – replacing the overlay as a button with a normal div.

renatho

Looks good and works well! I just needed to tweak a little the original Sensei Pro branch to test. haha 🙂

I just left a final comment, but if has a purpose, feel free to just ignore it. 😉

assets/shared/components/modal/style.scss

aaronfc · 2022-07-28T12:36:40Z

I was doing some testing and I found that the new implementation breaks when opening the Media Library modal when adding, for example, an Image.

It breaks because, when the Media library our modal is unfocused (and closed) making the content of the Media Library modal not appear (not completely sure why).

I am now wondering if maybe we should go back to the overlay... what do you think, @renatho ?

In order to test the issue:

Add an Image.
Use the Media Library.
Media Library will not load.

renatho

Just to document our recent conversation while approving this:

As we talked, I think we should ignore this problem since we shouldn't have more than one modal open at the same time.

And on the editor side, we'll probably use the Popover component to keep aligned with the new design and avoid this issue.

Add Modal reusable component.

f0f957f

- Implementation based on the same concept of sensei-modal used in Contact Teacher. -

aaronfc requested a review from a team July 19, 2022 13:49

aaronfc marked this pull request as ready for review July 19, 2022 14:52

aaronfc added 2 commits July 19, 2022 17:01

Set customisable title for Modal.

8f2141c

Set fixed height for modal title.

416a3f1

alexsanford reviewed Jul 20, 2022

View reviewed changes

renatho reviewed Jul 21, 2022

View reviewed changes

Review improvements.

1308305

- Add jsdoc. - Simplify hidden modal by returning `null` instead of hiding with css class. - Receive close callback because modal won't be ever open from the modal itself. - Fix css for height.

aaronfc commented Jul 25, 2022

View reviewed changes

aaronfc requested a review from renatho July 25, 2022 07:25

aaronfc added 2 commits July 25, 2022 09:27

Update with trunk.

ad94407

Pass click event on handleClose callback for modal and css.

41d676a

fjorgemota assigned aaronfc Jul 25, 2022

fjorgemota reviewed Jul 25, 2022

View reviewed changes

assets/shared/components/modal/index.js Outdated Show resolved Hide resolved

assets/shared/components/modal/index.js Outdated Show resolved Hide resolved

assets/shared/components/modal/index.js Outdated Show resolved Hide resolved

aaronfc added 3 commits July 26, 2022 10:53

Remove closures and fix some styles for Twenty Twenty One.

036e3e3

Add support to modal close upon ESC key pressed.

ccb8fe1

Renaming handleClose to onClose for modal.

b489cda

aaronfc requested a review from fjorgemota July 26, 2022 09:02

aaronfc added 2 commits July 26, 2022 11:55

Fix height for modal title.

b442e73

Fix height finally.

3af965b

fjorgemota reviewed Jul 26, 2022

View reviewed changes

renatho reviewed Jul 26, 2022

View reviewed changes

aaronfc and others added 5 commits July 27, 2022 11:35

Inline classname.

b1e5cb9

Co-authored-by: Fernando Jorge Mota <fjorgemota@users.noreply.github.com>

Use ESCAPE constant.

a327b8b

Removing return not needed in jsdoc.

1d4574a

Adding aria-label to close button.

da7e396

aaronfc requested review from renatho and fjorgemota July 27, 2022 14:57

renatho previously approved these changes Jul 27, 2022

View reviewed changes

assets/shared/components/modal/style.scss Show resolved Hide resolved

Unsettingn outline for close button.

859a60c

aaronfc dismissed renatho’s stale review via 859a60c July 28, 2022 14:14

renatho approved these changes Jul 28, 2022

View reviewed changes

aaronfc merged commit 34e332a into trunk Jul 28, 2022

aaronfc deleted the add/sensei-modal branch July 28, 2022 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reusable modal component. #5367

Add reusable modal component. #5367

aaronfc commented Jul 19, 2022 •

edited

Loading

codecov bot commented Jul 19, 2022 •

edited

Loading

alexsanford left a comment

fjorgemota commented Jul 20, 2022

renatho left a comment

renatho Jul 21, 2022

aaronfc Jul 25, 2022 •

edited

Loading

aaronfc left a comment

fjorgemota left a comment •

edited

Loading

aaronfc commented Jul 26, 2022

fjorgemota left a comment •

edited

Loading

renatho left a comment

renatho Jul 26, 2022

renatho Jul 26, 2022

aaronfc Jul 27, 2022

renatho Jul 26, 2022

aaronfc Jul 27, 2022

renatho Jul 26, 2022

aaronfc Jul 27, 2022

renatho left a comment

aaronfc commented Jul 28, 2022

renatho left a comment

Add reusable modal component. #5367

Add reusable modal component. #5367

Conversation

aaronfc commented Jul 19, 2022 • edited Loading

Changes proposed in this Pull Request

Testing instructions

Discussion

codecov bot commented Jul 19, 2022 • edited Loading

Codecov Report

alexsanford left a comment

Choose a reason for hiding this comment

fjorgemota commented Jul 20, 2022

renatho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronfc Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

aaronfc left a comment

Choose a reason for hiding this comment

fjorgemota left a comment • edited Loading

Choose a reason for hiding this comment

aaronfc commented Jul 26, 2022

fjorgemota left a comment • edited Loading

Choose a reason for hiding this comment

renatho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho left a comment

Choose a reason for hiding this comment

aaronfc commented Jul 28, 2022

renatho left a comment

Choose a reason for hiding this comment

aaronfc commented Jul 19, 2022 •

edited

Loading

codecov bot commented Jul 19, 2022 •

edited

Loading

aaronfc Jul 25, 2022 •

edited

Loading

fjorgemota left a comment •

edited

Loading

fjorgemota left a comment •

edited

Loading