Feature: Added renderTo prop instead of appendToBody by pravton · Pull Request #45 · wethegit/react-modal

pravton · 2024-06-21T19:47:10Z

Description

This PR addresses issue #20 by removing the appendToBody prop and introducing the renderTo prop for the Modal component.

Solution

Added the renderTo prop, accepting both a string selector or an HTMLElement.
Removed the appendToBody prop.
If renderTo is a string, the modal will be appended to the element selected by document.querySelector. If it’s an HTMLElement, the modal will render in that element. If renderTo is not provided or fails, the modal defaults to document.body.

Additional Notes

This is a breaking change.

Screenshots

demo.mp4

…MLElement and updated test modals

… element

changeset-bot · 2024-06-21T19:47:13Z

🦋 Changeset detected

Latest commit: 6610bbf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@wethegit/react-modal	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

andrewrubin

Thanks @pravton!

Couple comments from me here, and some ideas which might help to solve the hydration issues we were discussing.

Let me know if any of my feedback isn't clear 🙏

andrewrubin · 2024-06-21T20:41:03Z

+const getModalRoot = (renderTo?: string | HTMLElement) => {
+  if (typeof window !== "undefined") {
+    if (typeof renderTo === "string") {
+      return document.querySelector(renderTo) || document.body


I think we need to add some error handling here. If a user is passing an invalid selector, they won't know why their modal is being appended to the body. Maybe we just error instead of falling back to document.body?

Oh yes, I guess an error would be more helpful than appending body, I will add some error handling here.

@pravton I think there may've been a misunderstanding here based on what I'm seeing from the example.
If you tweak the renderTo examples where there's a string value passed in, the modal still opens when I believe what @andrewrubin meant is that if the element is not found, no modal should be opened.

Thanks Harvey! The intension was to keep the render in-place regardless since we don't use typescript in most cases, but based on Marlon's suggestion, we should be good once we remove the string/query selector.

andrewrubin · 2024-06-21T20:43:59Z

+
+export function Modal({ renderTo, className, ...props }: ModalProps) {
+  const modalRoot = getModalRoot(renderTo)
+  const appendToBody = modalRoot ? modalRoot === document.body : false


I know modalRoot does a check, but document.body will always be undefined on SSR here. Could be part of the hydration issue you were describing.

I think we could also rename this appendToBody variable to be a little more indicative of what its purpose is (being used to determine whether the body locks scrolling)

Hmm yes, but when I tried to return null based on the client side logic, based on this https://nextjs.org/docs/messages/react-hydration-error because "there was a difference between the React tree that was pre-rendered from the server". So we should be fine if nothing is returned based on client side logic.

and that make sense, I will rename the appendToBody..

andrewrubin · 2024-06-21T20:48:04Z

 }

-export function Modal({ appendToBody, className, ...props }: ModalProps) {
+const getModalRoot = (renderTo?: string | HTMLElement) => {


It might be worth memoizing this value as modalRoot within the component itself, rather than declaring a helper function outside.

Fore sure, i will memorize this function within the component.

…pdated variable name

pravton · 2024-06-21T22:54:46Z

@andrewrubin I have updated based on your suggestions, let me know if this works or if anything needs to be addressed or any other suggestions.

rvno · 2024-07-03T21:56:59Z

+  const modalRoot = useMemo(() => {
+    if (!isClient) return null
+
+    if (typeof renderTo === "string") {


@andrewrubin @pravton Tested locally and things look fine, but this could potentially be an issue in the event that there's multiple elements on the page with the same selector (in this case, classname); unless we're looking to only have at max, 3 modals - one for each type (body, selector, hash) - emptying and populating content on open/close.

Thinking that we could have the string value query for an element by id or by data-modal-id - working on an example atm, but lmkwyt.

I think we should instead remove the option to pass a string and only accept an HTMLElement. It makes this more intention as the user will have to handle nullish values.

It also feel more reacty that way as you can use focus more on using refs.

Hmm, yeah that makes sense! I will refactor this to only accept HTML element and I agree, feels more reacty this way..

rvno · 2024-07-03T23:32:12Z

+const getModalRoot = (renderTo?: string | HTMLElement) => {
+  if (typeof window !== "undefined") {
+    if (typeof renderTo === "string") {
+      return document.querySelector(renderTo) || document.body


@pravton I think there may've been a misunderstanding here based on what I'm seeing from the example.
If you tweak the renderTo examples where there's a string value passed in, the modal still opens when I believe what @andrewrubin meant is that if the element is not found, no modal should be opened.

marlonmarcello

I think we should NOT default to document.body and make that parameter mandatory instead of optional. Rendering in place by default.

Also following my other comment, we shouldn't accept a string for this I think, just an HTMLElement.

We should also make positon: fixed default regardless and let the user handle a differerent position strategy if they want.

Rendering in place with position: fixed covers 90% of the use cases and avoids all these SSR issues.

These changes force this prop to be more intentional overall.

…nd only accept HTMLElement

…ed changes

pravton · 2024-07-23T20:18:41Z

@andrewrubin @marlonmarcello @rvno So I made few changes based on the comments above,

Removed string prop, and modal component now only accepts HTMLElement. I have also made this prop mandatory in ModalProps.
By default the modal will be rendered in place with position: fixed
Updated the documentation to reflect the new changes

andrewrubin

These updates are great @pravton 👏 I think it solves all the issues nicely. Couple final ideas/questions from me.

andrewrubin · 2024-07-23T21:02:49Z

I think it would be beneficial to show an example in main.tsx of appending a modal to the body, thoughts?

For sure, yeah that would be helpful! I will add an example.

…d anchor text

pravton · 2024-07-23T23:31:35Z

@andrewrubin So I have added an example on the main appending the modal to body and updated the text as I see fit, but let me know if that needs to be updated or the example.

marlonmarcello · 2024-07-24T00:06:34Z

Great work @pravton !

andrewrubin · 2024-07-24T00:20:10Z

okay! Looking great @pravton

Next step here is to bump the version using the yarn changeset command, and in the questionnaire just specify that it's a major version bump. I'd also like to get a beta release going so we can test this before releasing.

It looks like the docs for that are here: https://github.com/changesets/changesets/blob/main/docs/prereleases.md, but I'm tagging @marlonmarcello to see if he has some quick tips for us on this.

andrewrubin · 2024-07-25T23:39:53Z

Thanks for the version bump @pravton

Let's keep this open for now, we'll just need to publish a beta version before merging this, so that we can test. I think the easiest option is to do that locally. Will make a note to do this when I have a moment!

andrewrubin

@pravton , sorry mate, I didn't realize this PR was to a release candidate and not main. In that case, we're good to merge this!

After, would you mind making a PR to main, for the release candidate? I will do the beta release from there

The base branch was changed.

pravton added 4 commits June 17, 2024 19:17

WIP - Removed appendToBody, added renderTo prop to accept string | HT…

cd10788

…MLElement and updated test modals

WIP - Removed render to same parent, can use ref/selecter to pass any…

308d061

… element

WIP - Updated documentaion to reflect the changes

210f29c

WIP - added client side check to prevent ssr issues on next js

d618772

pravton requested review from andrewrubin and marlonmarcello June 21, 2024 19:47

andrewrubin requested changes Jun 21, 2024

View reviewed changes

WIP - moved getModalRoot inside component, added error handling and u…

0337d29

…pdated variable name

pravton requested a review from andrewrubin June 21, 2024 22:54

liamegan self-requested a review July 3, 2024 16:54

rvno reviewed Jul 3, 2024

View reviewed changes

marlonmarcello reviewed Jul 4, 2024

View reviewed changes

Comment thread src/lib/components/modal/modal.tsx

marlonmarcello requested changes Jul 4, 2024

View reviewed changes

pravton added 2 commits July 22, 2024 16:30

update(react-modal): WIP - removed string prop, made prop mandatory a…

2cd8e40

…nd only accept HTMLElement

udpate(react-modal): WIP - updated documentation to reflect the updat…

a71a040

…ed changes

pravton requested review from marlonmarcello and rvno July 23, 2024 20:18

andrewrubin requested changes Jul 23, 2024

View reviewed changes

update(react-modal): added an example of appending to body and update…

3744520

…d anchor text

pravton requested a review from andrewrubin July 23, 2024 23:31

marlonmarcello previously approved these changes Jul 24, 2024

View reviewed changes

update(react-modal): bumping the version using changeset

6610bbf

pravton dismissed marlonmarcello’s stale review via 6610bbf July 25, 2024 23:21

andrewrubin previously approved these changes Jul 26, 2024

View reviewed changes

andrewrubin changed the base branch from release-candidate/v3 to release/v3.0.0 July 30, 2024 03:05

andrewrubin self-requested a review July 30, 2024 03:06

andrewrubin approved these changes Jul 30, 2024

View reviewed changes

andrewrubin merged commit f34c250 into release/v3.0.0 Jul 30, 2024

andrewrubin deleted the feature/renderto-instead-appendbody branch July 30, 2024 03:06

Conversation

pravton commented Jun 21, 2024

Description

Solution

Additional Notes

Screenshots

Uh oh!

changeset-bot Bot commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

andrewrubin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pravton Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pravton commented Jun 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marlonmarcello left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pravton commented Jul 23, 2024

Uh oh!

andrewrubin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pravton commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marlonmarcello commented Jul 24, 2024

Uh oh!

andrewrubin commented Jul 24, 2024

Uh oh!

andrewrubin commented Jul 25, 2024

Uh oh!

andrewrubin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

changeset-bot Bot commented Jun 21, 2024 •

edited

Loading

pravton Jun 21, 2024 •

edited

Loading

marlonmarcello left a comment •

edited

Loading

pravton commented Jul 23, 2024 •

edited

Loading

andrewrubin left a comment •

edited

Loading