feat: persist pinned cards in URL with DeepLinkProvider #4221
Conversation
psybuzz
force-pushed
the
psybuzz-urlpins3
branch
3 times, most recently
from
d4be201 to
86c18d3
Compare
psybuzz
force-pushed
the
psybuzz-urlpins2
branch
from
cdead2f to
ebe623b
Compare
psybuzz
force-pushed
the
psybuzz-urlpins3
branch
2 times, most recently
from
b045107 to
f2938c0
Compare
psybuzz
force-pushed
the
psybuzz-urlpins3
branch
from
f2938c0 to
7ad061e
Compare
| }); | ||
|
|
||
| describe('getUnresolvedImportedPinnedCards', () => { | ||
| it('returns false if no card exists', () => { |
There was a problem hiding this comment.
test description is incorrect.
|
|
||
| const state = appStateFromMetricsState( | ||
| buildMetricsState({ | ||
| unresolvedImportedPinnedCards: [ |
There was a problem hiding this comment.
I'd feel a little more confident about the test if there were multiple pinned cards. Can that be done without getting too verbose?
There was a problem hiding this comment.
In this case, the test does cover 2 unresolved pinned cards here, so no action needed iiuc.
| let pinnedCards = null; | ||
| for (const {key, value} of queryParams) { | ||
| if (key === 'pinnedCards') { | ||
| pinnedCards = pinnedCards || extractPinnedCardsFromURLText(value); |
There was a problem hiding this comment.
Trying to understand why this isn't written as:
if (key === 'pinnedCards') {
pinnedCards = extractPinnedCardsFromUrlText(value);
break;
}
Is it because you're trying to gracefully handle scenarios where pinnedCards appears multiple times but the first appearance is somehow malformed?
There was a problem hiding this comment.
Nope, the intent was only to take the first instance, in case there are multiple.
I can switch to use break if that reads better.
There was a problem hiding this comment.
Yes I think it would read and behave better if we used break;. As written it did cause a bit of head scratching.
| const result = []; | ||
| for (const item of object) { | ||
| // Validate types. | ||
| const isPluginString = typeof item.plugin === 'string'; |
There was a problem hiding this comment.
Nit: For consistency should one of isRunString or isTagTypeValid be renamed? They do the same check but are named differently.
| if (!item.tag) { | ||
| continue; | ||
| } | ||
| if (isRunString && (!item.runId || !isSingleRunPlugin(item.plugin))) { |
There was a problem hiding this comment.
I'm having a little bit of a hard time following what is happening on this line. I think it is two different error cases?
- runId is specified but is empty
- runId is specified and is non-empty but the plugin does not support runId being specified because it assumes multiple runs?
Maybe refactor and/or add a comment?
(On the other hand, the validation for sampleNumber is a bit more verbose but easier to read).
There was a problem hiding this comment.
True, these are 2 cases. Refactored a bit with comments.
| }); | ||
| }); | ||
|
|
||
| it('sanitizes pinned cards on deserialization', () => { |
There was a problem hiding this comment.
My preference would be that each of these is there own test case for several reasons: It's best practice to avoid control structures in tests; A particular test failure would be easier to identify by line number; tests would be easier to debug (wouldn't have to wait for any particular test cases's turn in the for loop).
It wouldn't be very much more verbose, if at all.
There was a problem hiding this comment.
But I see now this exact use case ("data-driven testing") is the one exception to the advice not to avoid control structures in testing:
go/unit-test-practices?polyglot=java#control-structures
There was a problem hiding this comment.
I think the current pattern fits "data-driven testing", though I can see why we'd want to split them up if the test had even a bit more complexity. Will keep as is, but I'll keep this in mind in the future.
Diffbase: #4221 Followup: #4245 Introduces an alert snackbar UI, which surfaces application errors to the user in the corner of the screen. App, feature modules may now register actions that trigger an alert on screen: ``` AlertActionModule.registerActionAlerts([ {localizedMessage: "Fetch failed"}, ... ]) ``` This change hides away the composite action using Angular's DI framework. The actions that should trigger alerts are registered once, and are dispatched by the AlertEffects, rather than specific features.
3 participants
Diffbase: #4220
When cards are pinned/un-pinned in Time Series, their
state is now reflected in the URL, while the user is on a
route of kind EXPERIMENT or COMPARE_EXPERIMENTS.
Pinned cards will persist across tab reloads. This behavior
can be configured per-route via updating the use of
CoreDeepLinkProvider.
Googlers, see test sync cl/335898836