feat: Add Modal component #309

bkonuwa · 2021-03-29T12:03:07Z

No description provided.

pixelbandito

Sorry, lot of words.
I checked out the code and overall, I'm really happy with how it looks and functions!

So please bear with my nitpicks, and feel free to push back on any of them.

pixelbandito · 2021-03-29T16:44:48Z

src/components/Modal/Modal.tsx

+    }
+  }, [isOpen]);
+
+  const modalContentStyle = useMemo(


I suspect this doesn't need to be memoized because:

The amount of processing it's doing is trivial

Changes to its output don't cause re-renders of large trees of react components below

pixelbandito · 2021-03-29T16:45:58Z

src/components/Modal/Modal.tsx

+      ),
+    [passedClassName, size]
+  );
+  return ReactDOM.createPortal(


I don't know if there's a catch, but on initial testing, this seems much cleaner than how we were handling the portals in the tk-ui modals. 👏

Yeah, I'm not sure there's a catch. That's how portal is used in this react TS cheet sheet. https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/portals/

example/src/App.js

pixelbandito · 2021-03-29T16:52:05Z

src/components/Modal/Modal.module.css

@@ -0,0 +1,129 @@
+.modal {


In the RoverUI convention, the main class for a component uses the same casing as the component name, so "Modal" here.

pixelbandito · 2021-03-29T16:52:42Z

src/components/Modal/Modal.module.css

+  opacity: 0;
+}
+
+.modalContent {


Considering we're already in a file named "Modal.*", maybe this should just be .content

pixelbandito · 2021-03-29T17:16:48Z

src/components/Modal/Modal.tsx

+        <div
+          role="presentation"
+          className={modalContentStyle}
+          onClick={(e) => e.stopPropagation()}


There's always a chance that a consumer will have a good reason for wanting to capture clicks inside the modal, from a parent.

Instead of using stopPropagation, you could handle this use case with a handleClickBackground function.
That function would look at the click event's target, and only call onClose if the event target is not inside the modal.

src/components/Modal/Modal.tsx

pixelbandito · 2021-03-29T17:38:38Z

src/shared/variables.css

@@ -25,4 +25,5 @@

  /* Borders */
  --rvr-border-radius: 4px;
+  --rvr-tile-border-radius: 6px;


This is nitpicky, but I'd rename this one, twice. Here's why:
For any css vars with multiple values (like border-radius, now), the convention should be

Have an ordered list of the values that doesn't use any semantic names. Think of this like a color palette.

Then add a semantic section that maps those values to their use cases.

In this case, it would be something like:

/* Palette */ --rvr-border-radius-md: 4px; --rvr-border-radius-lg: 6px; /* Semantic-ish */ --rvr-border-radius-tile: var(--rvr-border-radius-md);

That lets us change the whole feel of the app by changing the "palette" values, but it also gives developers a cheat sheet to know what size we typically use for tile-type components.

pixelbandito · 2021-03-29T17:43:43Z

src/shared/variables.css

@@ -25,4 +25,5 @@

  /* Borders */
  --rvr-border-radius: 4px;
+  --rvr-tile-border-radius: 6px;


Separate naming thing:
I think I would label this border radius with "dialog" instead of "tile"

There's no tight definition, but I found the breakdown on this page helpful. https://ux.stackexchange.com/questions/78798/whats-the-difference-between-cards-panels-and-tiles

The article suggests that "tile" is mostly associated with those square in Microsoft's Metro UI, and those aren't like modals at all.

I don't think modals quite fit into any of those categories, but I can imagine we'd end up with a few types of dialog box that should share the same border radius.

bkonuwa · 2021-03-30T03:01:17Z

Sorry, lot of words.
I checked out the code and overall, I'm really happy with how it looks and functions!

So please bear with my nitpicks, and feel free to push back on any of them.

No worries. Thanks for the feedback. I will add these suggestions this week.

…hat is expected in the UI

…rap tests, upgrade testing-library packages

pixelbandito

🌹 🍔

src/components/Modal/Modal.test.tsx

* feat: Add Modal component (#309) * feat: Add Modal component * cleaned up css class names * add logic to auto focus and focus trapping in the modal * Add example of Modal usage * update unit test by adding snapshot to verify the component renders what is expected in the UI * fix: Modal focus trap works better with keydown listener, add focus trap tests, upgrade testing-library packages * fix: use userEvent.type to simulate escape key press * fixed failing test Co-authored-by: Boima Konuwa <boima.konuwa@cision.com> Co-authored-by: Chris Garcia <pixelbandito@gmail.com> * feat: EVER-13212: a11y colors for buttons and text links Co-authored-by: bkonuwa <bkonuwa@gmail.com> Co-authored-by: Boima Konuwa <boima.konuwa@cision.com>

* Feature ever 13205 a11y buttons and links (#311) * feat: Add Modal component (#309) * feat: Add Modal component * cleaned up css class names * add logic to auto focus and focus trapping in the modal * Add example of Modal usage * update unit test by adding snapshot to verify the component renders what is expected in the UI * fix: Modal focus trap works better with keydown listener, add focus trap tests, upgrade testing-library packages * fix: use userEvent.type to simulate escape key press * fixed failing test Co-authored-by: Boima Konuwa <boima.konuwa@cision.com> Co-authored-by: Chris Garcia <pixelbandito@gmail.com> * feat: EVER-13212: a11y colors for buttons and text links Co-authored-by: bkonuwa <bkonuwa@gmail.com> Co-authored-by: Boima Konuwa <boima.konuwa@cision.com> * docs: Update changelog, publishing.md * Merge new features from v3.x to v4-alpha (#339) * Feature: EVER-13451: Add `Select` component (#325) * feat: EVER-13451: Initial commit of Select and Select.Option components * WIP: Keyboard handler for moving selected option focus with up arrow. * feat: EVER-13451: Up and down arrow keys, validity with hidden native select, break some code into separate hooks * feat: EVER-13451: Add tests for , make a child, export from index and import into example app * feat: EVER-13451: Update jsdom env to sixteen * feat: EVER-13451: Restore non-clicky style to plain Dropdown.Menu.Item content, style stories a bit * feat: EVER-13451: Update Dropdown.Menu.Item README * chore: EVER-13451: Update more tests and snapshots * v3.1.0 (#338) Just publishing a new minor version, no diff except the changelog and package.json version number. * Deploying v4.0.0-alpha.2 Co-authored-by: Chris Garcia <pixelbandito@gmail.com> Co-authored-by: bkonuwa <bkonuwa@gmail.com> Co-authored-by: Boima Konuwa <boima.konuwa@cision.com>

feat: Add Modal component

7805321

bkonuwa requested review from dkordik, pixelbandito, wendigolabs, mikemfleming and nvmusoke March 29, 2021 12:03

pixelbandito reviewed Mar 29, 2021

View reviewed changes

bkonuwa and others added 5 commits April 18, 2021 11:32

cleaned up css class names

8050d17

add logic to auto focus and focus trapping in the modal

67e39c7

Add example of Modal usage

46f2476

update unit test by adding snapshot to verify the component renders w…

a69b14b

…hat is expected in the UI

fix: Modal focus trap works better with keydown listener, add focus t…

5a2c251

…rap tests, upgrade testing-library packages

pixelbandito approved these changes Apr 27, 2021

View reviewed changes

dkordik requested changes Apr 28, 2021

View reviewed changes

src/components/Modal/Modal.test.tsx Outdated Show resolved Hide resolved

bkonuwa added 2 commits May 3, 2021 07:54

fix: use userEvent.type to simulate escape key press

92037a3

fixed failing test

e03f45f

dkordik approved these changes May 3, 2021

View reviewed changes

Merge branch 'master' into ROVERUI/Issue-296/Modal-Component

a67e19c

bkonuwa merged commit 144c622 into master May 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Modal component #309

feat: Add Modal component #309

bkonuwa commented Mar 29, 2021

pixelbandito left a comment

pixelbandito Mar 29, 2021

pixelbandito Mar 29, 2021

bkonuwa Mar 30, 2021

pixelbandito Mar 29, 2021

pixelbandito Mar 29, 2021

pixelbandito Mar 29, 2021

pixelbandito Mar 29, 2021

pixelbandito Mar 29, 2021

bkonuwa commented Mar 30, 2021

pixelbandito left a comment

feat: Add Modal component #309

feat: Add Modal component #309

Conversation

bkonuwa commented Mar 29, 2021

pixelbandito 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

Choose a reason for hiding this comment

bkonuwa commented Mar 30, 2021

pixelbandito left a comment

Choose a reason for hiding this comment