Custom domain feature ground work #326
Conversation
| const [isConnected, setIsConnected] = useState(false); | ||
| const [defaultSwitchState, setDefaultSwitchState] = useState(true); | ||
| const [cname, setCname] = useState('Todo.com'); | ||
| const [showForm, setShowForm] = useState(false); |
There was a problem hiding this comment.
Even without the connecting the cname functions, this is a very big component/page/section.
Break this up so this component isn't handling too much state. Yes you may pass in the state down to a few components for display.
This component does a few things.
1 shows/hides the form via switch
2. Connects the cname
3. Disconnects the cname
4. Handles showing/hiding disconnecting modal
An obvious one would be breaking up the show/hide toggle into a wrapper component. -- just to get rid of some state.
The next would be the Cname functionality, maybe have connecting and disconnecting separate components? -- disconnect would handle the modal? isConnected would be passed in.
^^ open to your thoughts on division.
Since these are big state holding components, I'd make their own file in the same directory as this page.
Make each component only handle one thing maybe 2 things. Save per component space, shrink components, shrink complexity.
There was a problem hiding this comment.
From discord, concise writing:
Main concern: I'd break this component up a lot more into new component files in the same directory as this component -- since it's this page specific
- Show hide switch toggle as a wrapper
- Connect Cname component
- Disconnect Cname component with the show/hide disconnect cname modal
| color="currentColor" | ||
| classProp="mr-16" | ||
| /> | ||
| Domain Varified |
There was a problem hiding this comment.
| Domain Varified | |
| Domain Verified |
| </ol> | ||
|
|
||
| {isConnected ? ( | ||
| <div className="flex-justify-"> |
There was a problem hiding this comment.
flex-justify- is this a class ending in a dash?
| placeholder="Hosted Domain" | ||
| required={true} | ||
| name="hosted_domain" | ||
| onChange={() => {}} |
There was a problem hiding this comment.
Make this a named "noop" function so we can find all the "noop" references and we can change them to actual functions when the time comes and we don't miss any.
| </FadeIn> | ||
|
|
||
| {showModal && ( | ||
| <ConfirmModal |
| onCancel, | ||
| }: ConfirmModalPropsT) => { | ||
| return ( | ||
| <Modal onClose={() => {}}> |
There was a problem hiding this comment.
Make named noop (unless it's actually supposed to do nothing.)
There was a problem hiding this comment.
this is whole file is just a noop, just layout to come back to
| @@ -10,7 +10,7 @@ | |||
| font-size: rem($size); | |||
| } | |||
|
|
|||
| @mixin primary-font($weight: 600) { | |||
| @mixin primary-font($weight: 500) { | |||
There was a problem hiding this comment.
double check is this supposed to be in here? -- no need to reply, just resolve if yes
There was a problem hiding this comment.
Since this can have side effects for other designs
| placeholder="Cname Record" | ||
| name="cname_record" | ||
| onChange={() => {}} | ||
| onBlur={() => {}} |
There was a problem hiding this comment.
whole file is a noop
None of the code on this PR ( except primary font change ) is visible on the live site. This is just the ground work for the domain connection form. After talking with Product we have decided to push this feature back a couple weeks in lieu of a more simpler version then we will come back to this form shortly and I don't want this code to get stale.