[lib] introduced components/modal to lib
AbandonedPublic
Actions

Authored by ginsu on Oct 12 2022, 12:29 PM.

Details

Reviewers

atul
rohan
• abosh
tomek

Summary

introduced components directory, and modal.react.js, modal.css, and modal-provider.js files to lib. I have a strong feeling that once D5315 gets approved, I will need to bring that logic into the modal.react.js file as well. Thinking that will be an additional diff to this stack

Next Steps:

Refactor modal.react.js in web to use lib Modal component
Refactor modal-provider.js imports in web to use lib instead of web
Refacor modal.react.js in landing
Refactor modal-provider.js imports in landing to use lib instead of landing

Linear Task: ENG-2038

Test Plan

N/A

More testing will come when refactoring web, but all of this code was pretty much approved in this diff

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Oct 12 2022, 12:29 PM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptOct 12 2022, 12:29 PM

ginsu edited the summary of this revision. (Show Details)Oct 12 2022, 12:38 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, rohan, • abosh.

ginsu edited the test plan for this revision. (Show Details)

ginsu edited the summary of this revision. (Show Details)Oct 12 2022, 12:40 PM

Harbormaster completed remote builds in B12757: Diff 17525.Oct 12 2022, 12:41 PM

ginsu requested review of this revision.Oct 12 2022, 12:41 PM

ginsu added inline comments.Oct 12 2022, 12:51 PM

lib/components/modal-provider.react.js
1–68 ↗	(On Diff #17525)	Used the Modal provider in web as inspiration
lib/components/modal.css
1–13 ↗	(On Diff #17525)	Used the Modal styles on web as inspiration
lib/components/modal.react.js
1–54 ↗	(On Diff #17525)	Used the Modal Component on web as inspiration

repositioned useLayoutEffect to be under useRef

ginsu added a child revision: D5354: [web] refactored modal.react.js in web to use lib Modal component.Oct 12 2022, 1:16 PM

ginsu added inline comments.

lib/components/modal-provider.react.js
1–68	Used the Modal provider in web as inspiration
lib/components/modal.css
1–13	Used the Modal styles on web as inspiration
lib/components/modal.react.js
1–54	Used the Modal Component on web as inspiration

Harbormaster completed remote builds in B12759: Diff 17527.Oct 12 2022, 1:24 PM

atul added a reviewer: tomek.Oct 13 2022, 8:38 AM

It looks like we're introducing a new modified modal.react.js in lib/components that will eventually be used by both web and landing... but is "dead code" for now.

I think a better approach would be to start with a noop diff that moves the existing modal components in web to lib and adjusting imports to have things work as before.

If you think this is the best approach, would be good to explain the differences between the existing and new modal components.

lib/components/modal-provider.react.js
1–68	Can you flag any differences? As a reviewer I can pull up the files side-by-side and try to spot the differences (more likely use a diff tool), but it'd be way easier if the diff author handled that In this case looks like the only differences are whitespace/newlines?
lib/components/modal.css
1–13	Can you flag any differences? As a reviewer I can pull up the files side-by-side and try to spot the differences (more likely use a diff tool), but it'd be way easier if the diff author handled that In this case looks like the difference is using hex value for `background-color` instead of `rgba(...)`?
lib/components/modal.react.js
1–54	Can you flag any differences? As a reviewer I can pull up the files side-by-side and try to spot the differences (more likely use a diff tool), but it'd be way easier if the diff author handled that It looks like there are a lot of differences here. Why do we need to modify the modal components in `web` so much? Would it not be possible to just move the modal components from `web/modals` to `lib/components` and change imports?

This revision now requires changes to proceed.Oct 13 2022, 8:50 AM

ginsu requested review of this revision.Oct 13 2022, 12:20 PM

ginsu added inline comments.Oct 13 2022, 12:33 PM

lib/components/modal-provider.react.js
1–68	Only difference here is newlines/whitespace for better readabilty
lib/components/modal.css
1–13	Only difference is switching `background-color` to hex value from rgba
lib/components/modal.react.js
1–54	This component is pretty much just the dark overlay surrounding the modal. The reason why I didn't just copy and paste the same modal component from `web` is that the `web` modal has a bunch of other features that `landing` does not need, for instance, name, icon, and close button. All these extra features also use other dependencies and components that would also need to be brought over to `lib`. The big difference between the modal on `web` and this one here is: This uses `useLayoutEffect` and not `useEffect`. Reasoning here I stripped out all the unnecessary features from `web` including things like header icon, name, corner close button

In D5353#158422, @atul wrote:

It looks like we're introducing a new modified modal.react.js in lib/components that will eventually be used by both web and landing... but is "dead code" for now.

I think a better approach would be to start with a noop diff that moves the existing modal components in web to lib and adjusting imports to have things work as before.

Very much agree with @atul... this diff should be moving the file instead of introducing a copy. The approach you took here makes it hard to easily see the changes. Read more