-
Notifications
You must be signed in to change notification settings - Fork 46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good overall 👍🏼
extension/source/QuillPage/components/OnboardingActionPanel.tsx
Outdated
Show resolved
Hide resolved
extension/source/QuillPage/components/ViewSecretPhrasePanel.tsx
Outdated
Show resolved
Hide resolved
extension/source/QuillPage/components/ViewSecretPhrasePanel.tsx
Outdated
Show resolved
Hide resolved
<div className="show-box"> | ||
<div style={{ display: 'inline-block' }}> | ||
<Button | ||
onPress={() => setExpanded(true)} | ||
highlight={false} | ||
icon={{ | ||
src: browser.runtime.getURL('assets/eye.svg'), | ||
px: 19, | ||
}} | ||
> | ||
Show secret phrase | ||
</Button> | ||
</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div className="show-box"> | |
<div style={{ display: 'inline-block' }}> | |
<Button | |
onPress={() => setExpanded(true)} | |
highlight={false} | |
icon={{ | |
src: browser.runtime.getURL('assets/eye.svg'), | |
px: 19, | |
}} | |
> | |
Show secret phrase | |
</Button> | |
</div> | |
</div> | |
<QuickRow> | |
<div style={{ display: 'inline-block' }}> | |
<Button | |
onPress={() => setExpanded(!expanded)} | |
highlight={false} | |
icon={{ | |
src: browser.runtime.getURL('assets/eye.svg'), | |
px: 19, | |
}} | |
> | |
{expanded ? "Hide secret phrase" : "Show secret phrase"} | |
</Button> | |
</div> | |
{expanded && <Button | |
onPress={() => {}} | |
highlight={true} | |
icon={{ | |
src: browser.runtime.getURL('assets/arrow-small.svg'), | |
px: 19, | |
}} | |
> | |
Review secret phrase | |
</Button>} | |
</QuickRow> |
Can use a single button for showing/hiding the box. Might need to change the CSS as well.
display:none
in CSS still renders an invisible component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
display:none
in CSS still renders an invisible component
Does this change your view? Either way I feel these are premature optimizations and it's probably better to move forward than iron them out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change your view?
No, Not really. Also the discussion is from 2012.
The element must atleast be present in the DOM for CSS to be applied on it. If there is no element then there cant be any style.
Either way I feel these are premature optimizations
Yeah I do agree. However what i feel that once the code base grows big, these things will be hard to find. Better to take action on smaller things than leave them for later.
eg - this was a big enough change which could be delayed for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see you were using the word 'render' differently 👍.
I'm not convinced it matters but I also don't mind either way. Fixed.
Co-authored-by: Kautuk Kundan <kautukkundan@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
(I thought I had pending comments and was trying to troubleshoot it. Not sure what happened and now I can't delete this 🤷♂️.)
icon={<ArrowRight className="icon-md" />} | ||
> | ||
Review secret phrase | ||
</Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey this block is unnecessarily too long
This suggestion handles the same thing in much less lines of code, happy to discuss.
#93 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the hyperlink doesn't seem to work. Here's the code snippet -
(button need fixing though)
<QuickRow>
<div style={{ display: 'inline-block' }}>
<Button
onPress={() => setExpanded(!expanded)}
highlight={false}
icon={{
src: browser.runtime.getURL('assets/eye.svg'),
px: 19,
}}
>
{expanded ? "Hide secret phrase" : "Show secret phrase"}
</Button>
</div>
{expanded && <Button
onPress={() => {}}
highlight={true}
icon={{
src: browser.runtime.getURL('assets/arrow-small.svg'),
px: 19,
}}
>
Review secret phrase
</Button>}
</QuickRow>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by the hyperlink? If you mean the Review secret phrase
button, that's added in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey this block is unnecessarily too long
I've implemented this because I don't mind the combined button approach but I don't think there was anything wrong there. Less code doesn't mean better code. Reviews on the other hand benefit greatly if they can be made shorter.
Co-authored-by: Kautuk Kundan <kautukkundan@gmail.com>
What is this PR doing?
Adds a rough version of the onboarding ui with very little styling:
How can these changes be manually tested?
Click the
Get Started →
button on the popup to view the screens above (click the numbers).Does this PR resolve or contribute to any issues?
Contributes to #90.
Checklist
Guidelines
resolve conversation
button is for reviewers, not authors