The Wayback Machine - https://web.archive.org/web/20201022083402/https://github.com/atom/about/pull/8
Skip to content
This repository has been archived by the owner. It is now read-only.

Show THE SQUIRREL #8

Merged
merged 26 commits into from Jan 13, 2016
Merged

Show THE SQUIRREL #8

merged 26 commits into from Jan 13, 2016

Conversation

@joshaber
Copy link
Contributor

@joshaber joshaber commented Jan 12, 2016

Add the squirrel icon when an update is available.

See atom/atom#10377 for the discussion that prompted this.

/cc @atom/feedback

@maxbrunsfeld
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld commented Jan 12, 2016

I think it'd be good to add a spec for this little dude, similar to the one from release notes.

@joshaber
Copy link
Contributor Author

@joshaber joshaber commented Jan 12, 2016

Yeah definitely 👍

@joshaber joshaber changed the title Show THE SQUIRREL [WIP] Show THE SQUIRREL Jan 12, 2016
@joshaber joshaber changed the title [WIP] Show THE SQUIRREL Show THE SQUIRREL Jan 12, 2016
@joshaber
Copy link
Contributor Author

@joshaber joshaber commented Jan 12, 2016

This is ready for 👀 now.


dispatchCall = spyOn(atom.commands, 'dispatch')
$(workspaceElement).find('.about-release-notes').trigger('click')
expect(dispatchCall.mostRecentCall.args[1]).toBe 'about:view-release-notes'

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Jan 12, 2016
Contributor

Since the handling of the about:view-release-notes command is not described elsewhere in the spec, It might be nice to include that's command behavior in this test by spying on shell.openExternal instead of atom.commands.dispatch.

This comment has been minimized.

@joshaber

joshaber Jan 12, 2016
Author Contributor

Yeah I was on the fence about that. It felt a bit like testing an implementation detail, but I can test it if you prefer 👍

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Jan 12, 2016
Contributor

Yeah, a pretty minor distinction, I agree. My thought was that they're both implementation details, but the behavior of openExternal is at least defined outside of this package.

@joshaber joshaber changed the title Show THE SQUIRREL [WIP] Show THE SQUIRREL Jan 13, 2016
@joshaber joshaber changed the title [WIP] Show THE SQUIRREL Show THE SQUIRREL Jan 13, 2016
atom.commands.dispatch(atom.views.getView(atom.workspace), 'about:view-release-notes')
})

this.subscriptions.add(atom.tooltips.add(this.element, {title: 'An update will be installed the next time Atom is relaunched.<br/><br/>Click to view release notes.'}))

This comment has been minimized.

@joshaber

joshaber Jan 13, 2016
Author Contributor

@atom/feedback How do we feel about this message?

This comment has been minimized.

@joshaber
Copy link
Contributor Author

@joshaber joshaber commented Jan 13, 2016

Alright, I think this is really and truly ready 🙏

@lee-dohm
Copy link
Member

@lee-dohm lee-dohm commented Jan 13, 2016

Let's do this! 🚀

@maxbrunsfeld
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld commented Jan 13, 2016

Thanks for fixing this long-standing pain-point. ❤️

@joshaber
Copy link
Contributor Author

@joshaber joshaber commented Jan 13, 2016

🚢

joshaber added a commit that referenced this pull request Jan 13, 2016
Show THE SQUIRREL
@joshaber joshaber merged commit b6c9705 into master Jan 13, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@joshaber joshaber deleted the squirrel branch Jan 13, 2016
this.subscriptions = new CompositeDisposable()

this.on('click', () => {
atom.commands.dispatch(atom.views.getView(atom.workspace), 'about:view-release-notes')

This comment has been minimized.

@benogle

benogle Jan 13, 2016
Contributor

Should this show the about page then have a button on the about page that triggers the restart? This flow seems maybe not ideal to have the squirrel point to the external page. I would like to train users that the version / update info is on the about page.

This comment has been minimized.

@joshaber

joshaber Jan 13, 2016
Author Contributor

We could, though as a user I'm not sure what going to the About view gives me, besides another click to view Release Notes.

This comment has been minimized.

@benogle

benogle Jan 13, 2016
Contributor

It gives info about the version you're on, what is available and a button to restart, along with a release notes button. Similar to chrome.

This comment has been minimized.

@benogle

benogle Jan 13, 2016
Contributor

The goal is to have the about page be the sort of hub for version and update management stuff e.g. atom/atom#9164. Like when folks uncheck the new autoupdate option, we should provide a way to check for an update on the about page.

This comment has been minimized.

@joshaber

joshaber Jan 13, 2016
Author Contributor

Ah yeah, but it doesn't do any of that stuff currently. This is just a stop gap so we can stop getting a billion issue reports about release notes being wrong. But if you feel strongly I can change this to open About.

This comment has been minimized.

@benogle

benogle Jan 13, 2016
Contributor

It's ok for this release. With the removal of the release notes package, we are removing the 'restart and update' button. So people will need to manually quit and restart atom to get the update to take effect. I know we're close to releasing, so might be too much to ask to get that button added at this point. But it's something we should add back for the next release.

This comment has been minimized.

@joshaber

joshaber Jan 13, 2016
Author Contributor

Should we slow it down and not bring these changes into 1.4?

This comment has been minimized.

@benogle

benogle Jan 13, 2016
Contributor

I think we should continue with it. The upside is that we wont get any release notes issues. The issues might come in when we release 1.5 as folks might be confused that there is no restart button. Or maybe no one will care, and my worries are unfounded. In any case, I think this is better than what we had.

This comment has been minimized.

@joshaber

joshaber Jan 13, 2016
Author Contributor

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants
You can’t perform that action at this time.