Skip to content
This repository has been archived by the owner. It is now read-only.

Adding Additional Information to About Page #81

Merged
merged 12 commits into from Jul 10, 2018

Conversation

Projects
None yet
6 participants
@trevorrl
Copy link

trevorrl commented Jun 23, 2018

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.
  • All new code requires tests to ensure against regressions

Description of the Change

I've added the information provided via atom -v on the command line to the About page, as suggested here: atom/atom#17547

The about page already provides the Atom version, I simply use the process.versions helper to get the Electron and Chrome versions, and the process.version to get the Node version.

These are shown by clicking a Show more link directly below the existing atom version number, and are smaller in order to not clutter up the design.

Edit by @rsese to drop in before/after screenshots

Before:
before
After, collapsed:
after-collapsed
After, expanded:
after-expanded

Alternate Designs

I at first just had the new version numbers shown by default directly below the Atom version, in the same font size, and this looked cluttered and busy. I decided since it was extra info and only useful to a subset of people, I'd put it behind a Show more button. This prevents cluttering the view, but allows additional useful information if desired.

Benefits

This change will give people the ability to quickly view the current versions of Electron, Chrome and Node being used in their build of Atom without having to open a terminal, and is more discoverable than a terminal command.

Possible Drawbacks

I haven't noticed any drawbacks in my testing, and no changes in performance of the About page when opening or reloading. It doesn't hide any existing info or add steps to get to existing info, so those who don't need / want the additional version numbers provided won't notice a difference.

Applicable Issues

n/a

@trevorrl trevorrl closed this Jun 23, 2018

@trevorrl

This comment has been minimized.

Copy link
Author

trevorrl commented Jun 23, 2018

Just figured out how to actually run specs locally, so I'm going to make sure my newly written tests actually pass before reopening.

@trevorrl trevorrl reopened this Jun 23, 2018

@rsese

This comment has been minimized.

Copy link
Member

rsese commented Jun 28, 2018

Thanks for contributing @trevorrl 👍

The pull request template only needs to be filled out here as you've done above, we shouldn't edit the actual pull request template file (that's what shown for anyone that opens a new pull request). Can you remove those changes to PULL_REQUEST_TEMPLATE.md?

Also, can you share before and after screenshots?

Once that's done, though we can't make any promises about the pull request being accepted, I'll ask the team to take a look.

@trevorrl

This comment has been minimized.

Copy link
Author

trevorrl commented Jun 29, 2018

Thanks for taking a look! I reverted the pull request template in the last commit, and here are some screenshots.

Before:
before
After, collapsed:
after-collapsed
After, expanded:
after-expanded

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jul 3, 2018

@simurai Could you give some design feedback on this?

),
$.div({className: 'about-more-expand'}),
$.span({className: 'about-version-container inline-block show-more-expand', onclick: this.handleShowMoreClick.bind(this)},
$.span({className: 'about-more-expand'}, 'Show more')

This comment has been minimized.

@50Wliu

50Wliu Jul 4, 2018

Member

This text should change based on whether the extended information is shown or not.

This comment has been minimized.

@simurai

simurai Jul 6, 2018

Member

Or the "Show more" could disappear completely until you reopen the About page again?

The other version could also be a bit more subtle color: @text-color-subtle; and smaller font-size: .9em; in font size, maybe like:

image

$.span({className: 'icon icon-clippy about-copy-version'})
),
$.a({className: 'about-header-release-notes', onclick: this.handleReleaseNotesClick.bind(this)}, 'Release Notes')
),
$.div({className: 'about-more-expand'}),

This comment has been minimized.

@simurai

simurai Jul 6, 2018

Member

I think this div is not needed, it currently is empty.

@@ -47,6 +47,10 @@
max-width: 500px;
}

.hidden {
visibility: hidden;

This comment has been minimized.

@simurai

simurai Jul 6, 2018

Member

There is already a

.hidden {
  visibility: hidden; !important;
}

global Atom core rule, so this style has no effect and can be removed.

$.span({className: 'about-version-container inline-block show-more-expand', onclick: this.handleShowMoreClick.bind(this)},
$.span({className: 'about-more-expand'}, 'Show more')
),
$.div({id: 'show-more', className: 'show-more hidden about-more-info'},

This comment has been minimized.

@simurai

simurai Jul 6, 2018

Member

Should we avoid using ids to not clash with another show-more id somewhere else? Maybe we could use a more specific class name and have the package name prefixed. That would also be more consistent with the rest of the class names. For example about-show-more or about-version-details.

@trevorrl

This comment has been minimized.

Copy link
Author

trevorrl commented Jul 6, 2018

Thanks for all the feedback @50Wliu and @simurai! I've got some time today so I'll start looking at your suggested changes, will hopefully have another commit by the end of the day.

Edit:
That was quicker than expected! Clicking 'Show more' now has this screen:
image
And clicking 'Hide' takes you back to the original:
image

Let me know what you think!

Edit 2: It actually wasn't an issue with the specs, just extra semicolons. Fixed!

Trevor Lemeron added some commits Jul 6, 2018

Trevor Lemeron
- removed unnecessary div
- using querySelector and class instead of getElementByID
- removed redundant .hidden style
- modified style for expanded info
- now toggle text between 'Show more' and 'Hide' when clicked
Trevor Lemeron

@lee-dohm lee-dohm merged commit 0cf3896 into atom:master Jul 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jul 10, 2018

@daviwil Would you mind doing the version dance here?

@trevorrl Thanks so much for working with us on this one 🎉

@trevorrl

This comment has been minimized.

Copy link
Author

trevorrl commented Jul 10, 2018

@lee-dohm no problem! It was my first contribution to OSS, feels pretty good 😄

glad I got the experience the process.

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Jul 11, 2018

This has been shipped as part of about 1.10.0 and will be in Atom 1.30.0-beta0. Thanks again for your contribution! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.