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

[DDW-173] Submit forms on Enter key #981

Merged
merged 15 commits into from
Jun 21, 2018

Conversation

thedanheller
Copy link
Contributor

@thedanheller thedanheller commented Jun 18, 2018

This PR enables submitting forms on Enter key press.

Todo:

  • Submit forms on Enter key
  • Enable it in the Confirm Transaction dialog
  • Add the :focus state style to all buttons
  • Replace onMouseUp to onClick on buttons, so it can be triggered by pressing Enter
  • Define the buttons :focus state style and colors for the 3 Daedalus themes (@a-rukin)

Screenshots

image
image
image
image
image


Review Checklist:

Basics

  • PR is updated to the most recent version of target branch (and there are no conflicts)
  • PR has good description that summarizes all changes and shows some screenshots or animated GIFs of important UI changes
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub
  • Automated tests: All acceptance tests are passing (npm run test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (npm run dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (npm run package / CI builds)
  • There are no flow errors or warnings (npm run flow:test)
  • There are no lint errors or warnings (npm run lint)
  • Text changes are proofread and approved (Jane Wild)
  • There are no missing translations (running npm run manage:translations produces no changes)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (npm run storybook)
  • In case of npm dependency changes both package-lock.json and yarn.lock files are updated

Code Quality

  • Important parts of the code are properly documented and commented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-rendering
  • Any code that only works in Electron is neatly separated from components

Testing

  • New feature / change is covered by acceptance tests
  • All existing acceptance tests are still up-to-date
  • New feature / change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review:

  • Merge PR
  • Delete source branch
  • Move ticket to done on the Youtrack board

@thedanheller
Copy link
Contributor Author

thedanheller commented Jun 18, 2018

@nikolaglumac probably not the best way to handle the "submit on enter" in the Send dialog.

@nikolaglumac
Copy link
Contributor

@daniloprates there are some flow/lint issues detected by the CI check.
Also, I don't think we need "Enter" key submission for this dialog.
Let's narrow down this feature just to the form input elements. E.g. only if some input is focused - only then it should be possible to submit by the enter key. Before we do it like that I would like to hear what @DominikGuzei thinks about it.

@thedanheller
Copy link
Contributor Author

@nikolaglumac it makes sense, although, from a user perspective, it feels right to have it, when submitting the previous form with Enter. Waiting for @DominikGuzei's opinion.

@nikolaglumac
Copy link
Contributor

nikolaglumac commented Jun 18, 2018

@daniloprates if that is what you want to have then we must have some kind of an active button state outline/glow on the active element - in this case the blue submit button.

@DominikGuzei
Copy link
Member

Yeah i agree with both of you - UX wise it would be nice to be able to submit all dialogs too with ENTER but then the submit button needs a glow / active state

@thedanheller
Copy link
Contributor Author

Cool, I'll give it a try

@nikolaglumac
Copy link
Contributor

The question is where to put an end to this feature - e.g. should the user also be able to use TAB key to switch between the fields and also change the focus of the buttons?

@thedanheller
Copy link
Contributor Author

thedanheller commented Jun 18, 2018

@nikolaglumac yes, I was thinking of changing my approach to a simple focus in the confirm button (along with a proper styling).

@nikolaglumac
Copy link
Contributor

@daniloprates OK let's see how it will look and feel ;)

@thedanheller
Copy link
Contributor Author

I like the solution. It feels good to use Enter when you've just used in the previous form.

image

The code is also much cleaner:
image

Looking forward to hear from you @nikolaglumac @DominikGuzei

@nikolaglumac
Copy link
Contributor

@a-rukin what do you think about the active button state? Do you like what @daniloprates is proposing or you have some better idea?

@alexander-rukin
Copy link
Contributor

Is this dotted line shown only when hovered by tab?

@thedanheller
Copy link
Contributor Author

@a-rukin nope, only in the :focus state.

@alexander-rukin
Copy link
Contributor

@daniloprates how this focus state can be achieved?

@alexander-rukin
Copy link
Contributor

alexander-rukin commented Jun 19, 2018

Also what happens on focus with grey button?

@thedanheller
Copy link
Contributor Author

@a-rukin this is the state where you can press Space or Enter to click the button. It can be achieve via javascript (<button autofocus ... and button.focus()) or when you mousedown a button and move the mouse arrow away from it.

I've chosen the light themes color based in the default Chrome outline color: #3b99fc. For the dark theme I've just chosen a lighter version of its background color. It's just a suggestion, feel free to choose more appropriate colors.

Bellow is how it looks in the dark theme:

image

@alexander-rukin
Copy link
Contributor

So if we press "Tab" button it should be the same, right?
Colors are fine

@Syeikhleiman
Copy link

Syeikhleiman commented Jun 19, 2018 via email

@DominikGuzei
Copy link
Member

@daniloprates i tested it and would like to have TAB either do nothing (keep the focus state on the submit button) or just switch between submit and cancel (within the dialog).

At the moment TAB removes the focus from submit and gives it to some other element (not obvious where) and only if you press TAB twice then you get focus back on the submit button (not so nice UX)

@thedanheller thedanheller removed the WIP label Jun 20, 2018
@thedanheller
Copy link
Contributor Author

@DominikGuzei

@alexander-rukin
Copy link
Contributor

41667241-d9b45f18-7482-11e8-88ca-623050a04c6f
Seems border lacks opacity

@thedanheller
Copy link
Contributor Author

@a-rukin that's right, I hadn't realized the opacity.

image

@alexander-rukin
Copy link
Contributor

Looks good!

Copy link
Member

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

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

Great work, love it! 🎉

Copy link
Contributor

@alexander-rukin alexander-rukin left a comment

Choose a reason for hiding this comment

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

OK

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

Looks and works great! 🎉

@nikolaglumac
Copy link
Contributor

Great work @daniloprates! We can merge this PR as soon as CI is done...

@nikolaglumac nikolaglumac merged commit d18c8ee into develop Jun 21, 2018
@nikolaglumac nikolaglumac deleted the feature/ddw-173-submit-forms-on-enter-key branch June 21, 2018 14:59
@ShimShamSam
Copy link

Why not just bind to the "submit" event of the form instead of listening for a keypress or mouse click individually?

@thedanheller
Copy link
Contributor Author

@ShimShamSam all forms had been done this way, so it was easier and safer to implement the "Enter" this way.

This was referenced Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants