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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further improvements to "empty" views/modals #1928

Merged
merged 11 commits into from Feb 6, 2019

Conversation

Projects
None yet
3 participants
@robertrossmann
Copy link
Contributor

robertrossmann commented Jan 31, 2019

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

This PR builds on #1927 and includes the following changes:

  • Tweaked the login flow to match the style of the reworked modals
  • Replaced git/github icons with more context-specific icons where appropriate (up to discussion)
  • Modals with buttons are now aligned to the pane's bottom instead of to the vertical centre of the pane (also up to discussion; views with no controls are still aligned to the centre)

    (reverted - all dialog views are now centered in the pane)

Screenshots follow. 馃帹

At the very least I would like to get the login modal rework merged; the rest is really up to discussion whether or not those changes are an improvement over the current look & feel.

Screenshot/Gif

Git tab

Too many changes
  • new icon

too many changes

Unsupported directory
  • new icon

unsupported directory

Create repository
  • new icon
  • aligned to bottom

create repository

Github tab

Select a Remote
  • new icon
  • aligned to bottom

select a remote

Login
  • added icons to both screens
  • aligned to bottom

login - 1
login - 2

Alternate Designs

I guess we can just keep the original Git & Github icons everywhere. I thought that using icons which better represent the current context would improve users' understanding of what's going on without reading the text. 馃

As for the alignment, I think keeping everything in the middle won't hurt but for some reason I feel like the buttons should be somewhere down. Maybe I'm biased from seeing too many mobile UI screens where these kinds of buttons tend to be down so as to be easily reachable with your thumb (which obviously does not matter here).

Screens with just a warning and no control should stay in the middle so as to catch your focus as quickly as possible. Screens with buttons are more easily perceivable due to strong colouring on those buttons.

All screens are now consistently aligned to the pane's centre. 馃帹

Benefits

  • Improved UX by showing contextual icons instead of generic git/github icon everywhere
  • For the login view, consistency with the other modals improved in #1927

Possible Drawbacks

N/A

Applicable Issues

N/A

Metrics

N/A

Tests

N/A

Documentation

N/A

Release Notes

Some of the less-frequently used screens (Github login, multiple remotes selection view etc.) have received visual overhaul. 馃帹

User Experience Research (Optional)

N/A

robertrossmann added some commits Jan 31, 2019

Replace all Empty views' icons with new ones
The idea is to convey the view's purpose/meaning by showing an appropriate icon.

The icons chosen do convey the intended meaning ToMe鈩笍 and so they are subject to discussion. 馃槆

@vanessayuenn vanessayuenn requested a review from simurai Jan 31, 2019

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Jan 31, 2019

Modals with buttons are now aligned to the pane's bottom instead of to the vertical centre of the pane

I love the other changes, but this one seems a little jarring. I would expect the dialogs to be consistently centered, and it also leaves a large amount of empty space above it.

@robertrossmann

This comment has been minimized.

Copy link
Contributor Author

robertrossmann commented Jan 31, 2019

@50Wliu Thanks for feedback! It just occurred to me that perhaps it would be better if we move the big icons consistently to the centre while preserving the buttons' placement?

Personally I like how the buttons "feel" when they are at the bottom - they seem to stick there regardless of viewport or pane size. If you align all the content to the middle the content seems to "travel" as you resize the viewport.

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Feb 1, 2019

It just occurred to me that perhaps it would be better if we move the big icons consistently to the centre while preserving the buttons' placement?

Here a screenshot:

screen shot 2019-02-01 at 1 08 47 pm

I like the idea of keeping the buttons in a predictable place.. but the single icon looks a bit weird. Maybe if the icon would be some bigger illustration?

An argument for keeping it centered might be that the distance to travel with your mouse and eyes is somewhat "averaged" and using the tabs wouldn't penalize you to move all the way to the bottom:

screen shot 2019-02-01 at 1 13 14 pm

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Feb 1, 2019

Another idea: Have 1-3 words wrapped with <strong> as a quick summary.

image

It's a bit like seeing a "404" on a website. After encountering this view a few times you might remember what it means and don't have to start reading from the start, you'll be like: "Ohh.. multiple remotes... ok!". The branch name would be wrapped with <code> instead.

Or have a title everywhere, like

image

robertrossmann added some commits Feb 1, 2019

@robertrossmann

This comment has been minimized.

Copy link
Contributor Author

robertrossmann commented Feb 1, 2019

@simurai Thanks for all the suggestions. I personally like the idea of having headings on all the views for quick summary of what's going on.

To somewhat address the concern of the buttons being too far away from the tabs, I added a significant margin to all the views with buttons to push the content up a bit - the views are still pushed down but they appear a little higher now (screenshots follow).

Also I increased the icons' size to make them more prominent.

Updated screenshots

create repo

multiple remotes

login - 1

login - 2

no remotes

Let me know your thoughts. If you still feel that the content should be centered I'll revert the placement back. Overall I like the other changes. 馃帹 馃槆 Thanks for feedback!

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Feb 4, 2019

If you still feel that the content should be centered I'll revert the placement back.

Yeah... if you don't mind? 馃槆 It feels a bit like a "visual glitch". As if there is an empty <div> above that pushes the rest down.

robertrossmann added some commits Feb 4, 2019

Align all modals with button controls to the pane's center
This partially reverts 890f08e in terms of code an fully reverts that commit in terms of intended behaviour.
Update heading on Remote Selector to use imperative tone
Previously the heading looked like an "error" message, however, this is really just a regular state that the user can appear in, and in future revisions this screen could be used for changing the remote directly from within the Github package, therefore an imperative tone fits better with the user's intent.
@robertrossmann

This comment has been minimized.

Copy link
Contributor Author

robertrossmann commented Feb 4, 2019

No problem @simurai, I have moved all the things to the pane's centre now. 馃挭

I have updated the PR's initial description (incl. screenshots) in case you'd like to review again.

I also noticed that all the other dialog views with buttons use an "imperative tone" in the headings except for the "Multiple Remotes" view, so I changed that heading to "Select a Remote" which I think better conveys the view's purpose. 馃

@simurai

simurai approved these changes Feb 6, 2019

Copy link
Member

simurai left a comment

Ok, this looks great. @robertrossmann thanks for all the improvements. 馃檶 馃檱

@simurai simurai merged commit 2c4b288 into atom:master Feb 6, 2019

@robertrossmann robertrossmann deleted the robertrossmann:feat/improve-empty-views branch Feb 6, 2019

@robertrossmann

This comment has been minimized.

Copy link
Contributor Author

robertrossmann commented Feb 6, 2019

Thanks for all the feedback and review! 鉂わ笍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.