Skip to content
This repository has been archived by the owner on Nov 5, 2023. It is now read-only.

Rough onboarding flow part 2 #95

Merged
merged 13 commits into from
Jan 27, 2022
Merged

Rough onboarding flow part 2 #95

merged 13 commits into from
Jan 27, 2022

Conversation

voltrevo
Copy link
Collaborator

@voltrevo voltrevo commented Jan 13, 2022

Dependent PR

This PR builds on top of #93. Please review and merge that first.

What is this PR doing?

Finishes the (mostly) unstyled onboarding flow by adding the confirmation phase for the secret words.

How can these changes be manually tested?

Click through the review phase.

Does this PR resolve or contribute to any issues?

Resolves #90.

Checklist

  • I have manually tested these changes
  • Post a link to the PR in the group chat

Guidelines

  • If your PR is not ready, mark it as a draft
  • The resolve conversation button is for reviewers, not authors
    • (But add a 'done' comment or similar)

@github-actions github-actions bot added the extension Browser extension related label Jan 13, 2022
Copy link
Contributor

@kautukkundan kautukkundan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor Changes

if (props.disabled) {
classes.push('disabled');
}

return (
<div
className={classes.join(' ')}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
className={classes.join(' ')}
className={`button ${highlight && "highlight"} ${loading && "loading"} ${disabled && "disabled"}`}

Will keep this component stateless

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to before, I don't like this because it inserts false.

I think this pattern comes up a lot though, and I agree that the classes.join approach is a bit verbose. How about a utility like this?

export default function classFlags(classes: Record<string, boolean>) {
  return Object.entries(classes)
    .filter(([, flag]) => flag)
    .map(([className]) => className)
    .join(' ');
}

That way we could write:

<div className={classFlags({ button: true, highlight, loading, disabled })}/>

Copy link
Contributor

@kautukkundan kautukkundan Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does look fine.
Also there's a popular one called classnames.

but again, it works only for this component as its static and the local variable doesn't change after this is rendered. In other components, conditional styles must set via state mutation and not local variable mutation.

Reason :
If the component's state changes, only the part where the state is consumed is re-rendered. However, in case local vars change in a component, ENTIRE component along with ALL the children are re-rendered as react sees it as a brand new component

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other components, conditional styles must set via state mutation and not local variable mutation.

I'm not sure why but I'm not able to follow what you're getting at here. Maybe an example where something similar to what I'm doing goes badly would help?

Copy link
Contributor

@kautukkundan kautukkundan Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apologies for not being clear, I meant that
state variables = variables declared using useState method
local variables = any other variable declared in the body of the component

Also I played around in a fresh project and I think that the reason I gave is wrong. Instead of re-rendering every child component, it wont do anything if a local variable is mutated.

Btw this doesn't apply in this particular case anymore, so I guess let's come back to this later if such a case arises again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@github-actions github-actions bot removed clients aggregator Aggregator backend related labels Jan 24, 2022
@voltrevo voltrevo marked this pull request as ready for review January 24, 2022 04:01
Copy link
Contributor

@kautukkundan kautukkundan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏼

Base automatically changed from bw-90-rough-setup-flow to main January 27, 2022 05:41
@voltrevo voltrevo merged commit 79f114a into main Jan 27, 2022
@voltrevo voltrevo deleted the bw-90-followup branch January 27, 2022 05:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extension Browser extension related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rough set-up flow
2 participants