Page MenuHomePhabricator
Differential D3451

[web] Turn `Splash` into a functional component
ClosedPublic
Actions

Authored by atul on Mar 16 2022, 3:41 PM.
Tags
None
Referenced Files
F3405971: D3451.diff
Tue, Dec 3, 11:57 PM
Unknown Object (File)
Wed, Nov 20, 4:31 PM
Unknown Object (File)
Sun, Nov 10, 12:13 AM
Unknown Object (File)
Sun, Nov 10, 12:13 AM
Unknown Object (File)
Sun, Nov 10, 12:13 AM
Unknown Object (File)
Sun, Nov 10, 12:13 AM
Unknown Object (File)
Sun, Nov 10, 12:13 AM
Unknown Object (File)
Sun, Nov 10, 12:07 AM
View All Files
Subscribers
• adrian
• karol

Details

Reviewers
tomek
• jacek
• benschac
varun
Commits
rCOMMd942b46e96fb: [web] Turn `Splash` into a functional component
Summary

This should be a noop.

(This component will be renamed from ConnectedSplash to Splash in the next diff).

depends on D3450

Test Plan

Careful reading, flow, and making sure we can still log into the web app, etc.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Mar 16 2022, 3:41 PM
Herald added subscribers: • karol, • adrian. · View Herald TranscriptMar 16 2022, 3:41 PM
atul published this revision for review.Mar 16 2022, 3:42 PM
atul updated this revision to Diff 10468.Mar 16 2022, 3:43 PM

rename

Harbormaster completed remote builds in B7437: Diff 10467.Mar 16 2022, 3:46 PM
Harbormaster completed remote builds in B7438: Diff 10468.Mar 16 2022, 3:51 PM
• benschac accepted this revision.Mar 16 2022, 5:24 PM
• benschac added inline comments.
web/splash/splash.react.js
12 ↗(On Diff #10468)

nit: Could we make this: SyntheticEvent<HTMLElement> so we don't have to update the type too if the element changes.

16 ↗(On Diff #10468)

Does the whole modal context need to be in the dep array?

32 ↗(On Diff #10468)

RIP it was fun while it lasted.

This revision is now accepted and ready to land.Mar 16 2022, 5:24 PM
atul edited the summary of this revision. (Show Details)Mar 16 2022, 9:12 PM
atul added a parent revision: D3450: [web] Remove everything from `Splash` except Log in button.
atul added 1 blocking reviewer(s): tomek.Mar 16 2022, 9:47 PM
This revision now requires review to proceed.Mar 16 2022, 9:47 PM
atul changed 2 blocking reviewer(s), added 1: ashoat; removed 1: tomek.Mar 17 2022, 8:13 AM
tomek accepted this revision.Mar 17 2022, 10:42 AM

Agree with Ben's comments

atul removed a reviewer: ashoat.Mar 17 2022, 10:59 AM
This revision is now accepted and ready to land.Mar 17 2022, 10:59 AM
atul updated this revision to Diff 10493.Mar 17 2022, 11:30 AM

rebase before addressing feedback and landing

Harbormaster failed remote builds in B7451: Diff 10493!Mar 17 2022, 11:31 AM
atul added inline comments.Mar 17 2022, 11:35 AM
web/splash/splash.react.js
12 ↗(On Diff #10468)

Ok sure, flow seemed to be happy with that. I didn't change it initially to keep it as minimal a refactor as possible, but doesn't hurt.

16 ↗(On Diff #10468)

ESLint wanted modalContext in the dep list and wasn't happy with modalContext.setModal

32 ↗(On Diff #10468)

rip

atul updated this revision to Diff 10494.Mar 17 2022, 11:36 AM

switch the type around

atul marked 2 inline comments as done.Mar 17 2022, 11:36 AM
Harbormaster failed remote builds in B7452: Diff 10494!Mar 17 2022, 11:37 AM
This revision was landed with ongoing or failed builds.Mar 17 2022, 11:37 AM
Closed by commit rCOMMd942b46e96fb: [web] Turn `Splash` into a functional component (authored by atul). · Explain Why
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMMd942b46e96fb: [web] Turn `Splash` into a functional component.

Revision Contents
Changeset List

PathSize
web/
splash/
46 lines

Diff 10467

View Options

web/splash/splash.react.js

Loading...