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

Move and refactor upgrade notification JS #5256

Closed
wants to merge 1 commit into from

Conversation

jonnyscholes
Copy link
Contributor

Part of #5252. This PR moves the upgrade notification JS to wagtail-client. I've re-written it for the most part, given it's simplicity including, removing its jQuery dependance.

Probably look at #5254 first because it raises some related questions.

Do the tests still pass? (http://docs.wagtail.io/en/latest/contributing/developing.html#testing) Yes.
Does the code comply with the style guide? (Run make lint from the Wagtail root) Yes.
For Python changes: Have you added tests to cover the new/fixed behaviour? N/A
For front-end changes: Did you test on all of Wagtail’s supported browsers?
Chrome 73 MacOS
Safari 12.0 MacOS
Firefox 66.0 MacOS
For new features: Has the documentation been updated accordingly? N/A

@squash-labs
Copy link

squash-labs bot commented Apr 24, 2019

Manage this branch in Squash

Test this branch here: https://neon-junglerefactor-js-upgrade-3hi0o.squash.io

Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

This looks awesome, so good to see this stuff getting migrated.
A few comments, nothing I can see that could cause major issues.

Other than the potential folder separation of components and DOM element modification 'components', this looks awesome.

* "url" : "https://wagtail.io" // Absolute URL to page/file containing release notes or actual package. It's up to you.
* }
*/
const releasesUrl = 'https://releases.wagtail.io/latest.txt';
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought - would there be any benefit in storing the releaseUrl on the container also?

benefits - can be customisable by devs without needing to load different JS (edge case probably), means the function itself contains less data hard coded (eventually the URL could be pulled out further into a tag or constant elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure. I think this is a good idea - off the top of my head I can see how this might be useful for CodeRed style projects.

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

This is looking good 🙂 @lb- @jonnyscholes is there anything else you’d want to see here or is it good to go as-is?

});
it('compares 2.6a0 and 2.4 correctly', () => {
expect(versionOutOfDate('2.6a0', '2.4')).toBeTruthy();
});
Copy link
Member

Choose a reason for hiding this comment

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

👏 thank you for the tests

@lb-
Copy link
Member

lb- commented Aug 4, 2019

This is good to go, I have merged it in with some resolved conflicts all sorted.

@jonnyscholes - merged in via c0af26b

I think there were two outstanding items, not blockers though.

  1. components folder housing both React and 'DOM / template style' components - maybe when more refactoring is done a good pattern will emerge and we can move things around then.
  2. slightly easier future customisation of putting the releases URL in the template, so it can be accessed by its data attribute. I think that the original code did not have this so all good, plus there are other ways to override what renders in the settings bar.

@lb- lb- closed this Aug 4, 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.

None yet

3 participants