Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New login screen to make non-master ticket logins easier to navigate #409

Merged
merged 9 commits into from
Mar 10, 2020

Conversation

jyng
Copy link
Contributor

@jyng jyng commented Mar 10, 2020

Screen Shot 2020-03-09 at 7 40 59 PM

image

Screen Shot 2020-03-09 at 7 40 17 PM

@jyng jyng requested review from vvisigoth and Fang- March 10, 2020 02:43
@matildepark
Copy link
Contributor

matildepark commented Mar 10, 2020 via email

@galenwp
Copy link

galenwp commented Mar 10, 2020

Yeah 'etc...' seems a bit odd. 'Other options (Metamask, Ledger, etc)' or similar.

@vvisigoth
Copy link
Contributor

vvisigoth commented Mar 10, 2020 via email

@vvisigoth
Copy link
Contributor

Otherwise, i think this is vast improvement, thnx. Approved based on changing ellipsis.

@jyng
Copy link
Contributor Author

jyng commented Mar 10, 2020

Edited

Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

This looks good @jyng. (:

onClick={() => setisOther(false)}
full
className="t-center underline f6 mt8">
Back
Copy link
Member

Choose a reason for hiding this comment

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

Should this just say "Master Ticket" or similar instead, to have more symmetry with the "Metamask, Mnemonic..." button?

Comment on lines +83 to +86
<Grid.Item full onClick={goToActivate} className="mv10 t-center f6">
<span className="gray4">New Urbit ID? </span>
<LinkButton>Activate</LinkButton>
</Grid.Item>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we don't just show this on both pages (ie regardless of isOther), but I can see how it's only strictly needed for the landing conditions.

(Also the space after "ID?" bother me. (^: )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this crossed my mind, but I felt like this page is for advanced users who are familiar with crypto. On the flipside, we haven't done a good job of getting people to use Master Ticket (even if they bought a planet from a third-party)

full
as={SubmitButton}
center
handleSubmit={handleSubmit}>
{isWarning =>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is as good a time as any to tear this out. See also #405. (But we might want further discussion on that.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah while I was styling this, I felt like it was an unnecessary layer of abstraction, but what do I know lol

@jyng jyng merged commit dc70bbd into master Mar 10, 2020
@Fang- Fang- deleted the jy/login-screen branch March 10, 2020 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants