Order channels by track/risk/branch #1581
Conversation
Codecov Report
|
|
Starting demo at: https://snapcraft-io-canonical-websites-pr-1581.run.demo.haus/ |
Lukewh
force-pushed
the
metrics-updates
branch
from
a2ae4e6
to
839555a
Compare
|
Works well. One thing I noticed, that is a bit related, but I'm not sure if we can fix this - is the ordering of sections on the graph itself. Currently it's different from the order on the tooltip which is a bit confusing - especially that the 'bold' on tooltip that highlights current channel is not very visible. Hovering over first top section (green) - first 10/stable channel is highlighted: Hovering over second from top (red) - way below 11/stable is highlighted: Should the order on the graph be the same as in the tooltip? Can we even control it? |
| risk: false, | ||
| branch: false | ||
| }; | ||
| const channelArr = ["latest", undefined, "_base"]; |
There was a problem hiding this comment.
Is there any reason for doing this in array instead of 3 variables (track, risk, branch)?
Also isn't the default branch empty? If we start using "_base" we may end up detecting a branch when there isn't any. But maybe as we approach adding support for branches in more elements of UI we should just start using "_base" as default. If that's the case it would be good to have "_base" (but "latest" as well) defined somewhere as consts and reused instead of being copied.
There was a problem hiding this comment.
So I introduced the "_base" as a concept to have both a common format for all track/risks, regardless of whether they have branches and also to help with parsing in other parts in this file, for example reducing/ sorting ["stable/test", "stable"] was producing ["stable/test"] so stripping the "base" stable channel.
Setting "_base" hopefully makes it clear for anyone calling this code in the future (for another part of the site)
| * ... | ||
| * | ||
| * @param {[string]} channels An array of "risk" / "track/risk" / "track/risk/branch" | ||
| * @param {{defaultTrack: string}} options |
There was a problem hiding this comment.
If there is no other option then defaultTrack maybe it just could be a defaultTrack param?
There was a problem hiding this comment.
I wanted to make it generic for future and be more descriptive in the code that calls it. I find:
sortChannels([], "randomString") harder to parse than sortChannels([], { defaultTrack: "randomString" }) when scanning code... I'm open to arguments against though!
There was a problem hiding this comment.
Not sure if "randomString" would be used in such way directly in code. I would guess in most cases default track would be passed as a variable, so it would be something more like sortChannels([...], defaultTrack) which would be more clear.
But, I agree that unnamed option params may be confusing (especially when passing booleans), so I'm not against for sure.
In this case I just wondered if there is any reason to pass it via object. And because it seems like it's used like that consistently in at least 2 functions I'm fine with leaving it as it is.
Code is clear to read, and works, so not worth spending time changing it for little benefit.
|
@bartaz thinking about your ordering comment some more: We could alter the order to match but the current implementation ensures that any new channel/ version/ os appears from the bottom to take a share of those above. Changing the order in the specific case you've shown would work (as they're all established channels) but if a new "latest/stable/test" was released, where should it appear? I don't think it's worth the extra effort/ code/ calculations, but I could easily be wrong :D |
|
@Lukewh Sure, never mind then. This issue is about order in the tooltip anyway. |
Fixes #1475
It also updates a few things.
QA
./run