-
-
Notifications
You must be signed in to change notification settings - Fork 891
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
Re-ordered targets for flasher. #1820
Re-ordered targets for flasher. #1820
Conversation
8fd3832
to
38f88fa
Compare
Can we put a pin in that one? If we move away from jQuery we might need to rework it anyways. Embedding a bunch of data into the DOM seemed dirty to me. |
After the experience I had reworking the existing code I'd rather keep the momentum going and clean this up a bit, lest somebody will have to reverse engineer the mess that it is now again next time we change something.
It is not my preferred approach either, but:
|
dc01b63
to
df5cd58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me is ok!!
@@ -219,27 +216,23 @@ TABS.firmware_flasher.initialize = function (callback) { | |||
loadUnifiedBuilds(releases); | |||
}; | |||
|
|||
function checkOneVersionForUnification(version) { | |||
function supportsUnifiedTargets(version) { | |||
return semver.gte(version.split(' ')[0], '4.1.0-RC1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your code, but to me is a little strange continue supporting the RC versions and only starting with the RC1. But I suppose is needed to not break things...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can check tomorrow but at the time it seemed best. 4.1.0 is newer than 4.1.0-RC1 so if we specified 4.1.0 we wouldn't have had unified targets with any RC. Specifying something like gte 4.0.9999 would have worked but then it's not clear to the humans as to what is going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@McGiverGim: This is not so much about 'supporting 4.1.0-RC1', but about making sure that we are not trying to use custom defaults with any (released) versions that do not support this. And 4.1.0-RC1 is the first released version to support custom defaults.
src/js/tabs/firmware_flasher.js
Outdated
version.descriptor.date, | ||
versionLabel | ||
)) | ||
.css("font-weight", FirmwareCache.has(version.descriptor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again not your code, but I prefer to add a class and not a style, to maintain it in the css files.
4ffe9a9
to
e97fd5b
Compare
@McGiverGim: I made it all classy now. |
@Docteh: Well spotted, thanks for testing. Turns out I inadvertently removed some calls to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Actually both texts are loaded from the beginning in the UI in |
Yeah I've stared at this tab enough to misremember things. If we get lots of complaints later on we could make "Choose a Board first" the second option |
You might have seen it - the use of |
This moves the selection of Unified / legacy target from the board list into the version list:
This should address the main reason for user confusion, but there are a couple more things to be cleaned up and improved in the firmware flasher tab: