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

Allow dragging seed phrase during Confirm Seed Phrase #6557

Merged
merged 11 commits into from
May 7, 2019

Conversation

chikeichan
Copy link
Contributor

@chikeichan chikeichan commented May 3, 2019

seed-dnd

  • Implement feature
  • Add unit test
  • Fix/Update Integration test

@cjeria
Copy link
Contributor

cjeria commented May 3, 2019

  • The border-radius is gone? I think they should be added them back in since they are like buttons. We may just want to use our button state styles here. Could be added later.
  • Can a shadow be added on hover? I think that will add affordance to the drag feature
  • The button text is being pushed down as you can see by the screenshots. Maybe adjust the line-height css properties?

image

  • Same for the buttons
    image

cc @bdresser

@cjeria
Copy link
Contributor

cjeria commented May 3, 2019

@chikeichan I made a demo of the draggable feature for styling purposes. Using our current design system button styles to see how it feels and I think it could work. Clickable demo version here: https://codepen.io/cjeria/pen/dEbMwg

And a gif (ignore the horizontally expanding parent container and font family).

@chikeichan
Copy link
Contributor Author

chikeichan commented May 3, 2019

The border-radius is gone? I think they should be added them back in since they are like buttons. We may just want to use our button state styles here. Could be added later.

Not gone -border-radius was never there on seed phrase 😄 . I will add that in next commit.

Can a shadow be added on hover? I think that will add affordance to the drag feature

will fix in next commit

The button text is being pushed down as you can see by the screenshots. Maybe adjust the line-height css properties?

will fix in next commit

I made a demo of the draggable feature for styling purposes. Using our current design system button styles to see how it feels and I think it could work. Clickable demo version here:

Thanks for the demo! What does it mean for specific changes on this PR? I can see that there are:

  • animations
  • seed phrases are organized by column (similar to kanban style), which doesn't really work since seedphrase are ordered from left-to-right, top-to-bottom.
  • change seed word style to Secondary Button

@cjeria

danjm
danjm previously approved these changes May 6, 2019
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

Code looks good! I have not QA'd yet.

@cjeria
Copy link
Contributor

cjeria commented May 6, 2019

animations

It'd be great to add transitions/animations but if not easy, I wouldn't worry about it.

seed phrases are organized by column (similar to kanban style), which doesn't really work since seedphrase are ordered from left-to-right, top-to-bottom.

No need to change the order, keep the current left-to-right structure. Again, just using the demo for styling purposes.

change seed word style to Secondary Button

Good catch. I was using a combo of primary and secondary. Let's go with secondary button styles. I updated the demo to reflect the correction. Also a gif. Font family shoud be Roboto.

@cjeria
Copy link
Contributor

cjeria commented May 6, 2019

And as for the default state of the unselected words, let's go with a combo button style. I'll add to the design system as button-toggle. @chikeichan

image

image

@chikeichan
Copy link
Contributor Author

chikeichan commented May 6, 2019

dnd

@cjeria
Copy link
Contributor

cjeria commented May 6, 2019

Looks good! Approved!

@danjm danjm merged commit 5811285 into MetaMask:develop May 7, 2019
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.

3 participants