Rewind the rules table UI prototype #3079
Conversation
michellemounde
force-pushed
the
rewind-the-rules-table-ui-prototype
branch
from
af3c2c5
to
afb7539
Compare
michellemounde
requested review from
jcristau,
gbrownmozilla and
gabrielBusta
as code owners
There was a problem hiding this comment.
(I will be using conventional commits for this review to try to help improve clarity.)
This is really excellent work! I know there's quite a few comments I'm making, but for the most part these are small papercuts or edge cases. Overall, this is working very well, and in very good shape. You've also improved on the original prototype in a number of ways - very nice!
issue: The rules do not sort correctly when we've rewound. When I first rewind they do not appear sorted at all (I have rule 63, then 80, then 91). If I reload the page with the timestamp query parameter in the URL they are reverse from lowest to highest priority (it should be highest to lowest). (This happens with a filter applied, and without a filter applied).
issue: Clicking on the rules link at the top of the page drops the timestamp from the URL, but does not reset the view to show the current rules. It should either do both of these things, or neither.
todo: please update the anchors in the rule cards to include the timestamp. (This way we can share links to a particular rule at a particular point in time.)
todo: please disable or remove the "Add Rule" control on on ListRules when the table is rewound (the "Disable Updates" button can stay enabled - we want it be easily accessible at all times).
michellemounde
force-pushed
the
rewind-the-rules-table-ui-prototype
branch
from
c5efc11
to
086386a
Compare
…ges filter in rewind mode
michellemounde
force-pushed
the
rewind-the-rules-table-ui-prototype
branch
from
086386a
to
919940f
Compare
There was a problem hiding this comment.
I'm marking this as needing changes for the first issue noted below - but this looks very, very good overall - and is almost ready to land.
issue: The "Updates are currently disabled for the xxx yyy channel" banner is not consistently shown when updates are disabled and the rules are rewound. It seems to be shown when you first disable in rewound mode - but not shown if you refresh the page.
issue (non-blocking): I'm not sure if this is a regression or not, but loading a link with an anchor in it does not scroll to the right place. For example, loading https://localhost:9000/rules?product=Firefox&channel=beta×tamp=1575398460000#ruleId=525 ends up with the top of the rule card cut off:
(It looks like the controls at the top may be floating on top of the rule card?)
There was a problem hiding this comment.
It looks like there's still at least one issue with the "Updates are currently disabled..." state. With these steps it isn't shown:
- Select a product/channel
- Rewind the rules
- Disable updates for the product/channel
- Clear the rewind
This causes the "Updates are disabled" dialog to disappear. If I refresh the page it re-appears. I suggest doing some logging of all of the variables that control whether or not the EmergencyShutoffCard is shown to help debug this (eg: productChannelFilter, emergencyShutoffs, etc.).
Once you get this sorted out this will be ready to go!
There was a problem hiding this comment.
This looks good to me now. @gabrielBusta should review it as well.
Thanks for all your hard work on this @michellemounde!!
| className={classes.rewindPicker} | ||
| disableFuture | ||
| inputVariant="outlined" | ||
| label="Rewind to..." |
There was a problem hiding this comment.
What about "Rule database state as of YYYY/DD/MM HH:MM AM/PM"?
There was a problem hiding this comment.
I think rewind to might be more clear for the label when there is no rewind date selected.
Implementation of rewind the rules table UI prototype from old Balrog UI.