Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the navigation bar to handle link highlighting via component logic using Angular Signals instead of routerLinkActive. The changes are generally well-aligned with the repository's style guide promoting modern Angular features.
My review includes two main points:
- A style guide violation regarding the use of
effectto update another signal. While I understand the reason for this choice, it's worth noting. - A lack of unit tests for the new highlighting logic, which is a critical part of this feature.
Please see the detailed comments for suggestions.
| effect(() => { | ||
| // When user navigates to a section matching the nav button target | ||
| // set as the current highlighted section | ||
| const templateName = this.templateMetaService.templateName(); | ||
| const targetTemplates = this.targetTemplates(); | ||
| const updatedSection = targetTemplates.findIndex((name) => name === templateName); | ||
| if (updatedSection > -1) { | ||
| this.activeLinkIndex.set(updatedSection); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This effect updates the activeLinkIndex signal, which goes against the repository style guide.
This pattern is generally discouraged in Angular as it can lead to complex state management and potential issues like ExpressionChangedAfterItHasBeenChecked. The preferred way to derive state is using a computed signal.
I understand that you're using an effect here to achieve the "persistence" of the highlight when navigating to a route not in the navigation bar, which is difficult with a computed signal. If this is a deliberate and necessary trade-off, please consider adding a comment explaining why this approach was chosen over a computed signal to help future developers understand the context.
References
- Use
effectfor side effects that should run when a signal's value changes. ... Avoid using effects to update other signals. (link)
There was a problem hiding this comment.
@chrismclarke – as you highlighted in #3295 (comment) (and I agree), this feels like an overzealous application of the general rule to avoid updating signals in effects where possible. Obviously Angular has now made this behaviour the default, but it is still a pattern that is discouraged as far as I can tell, but in some cases it's clearly much better than the alternatives. Possibly worth updating our styleguide? Or maybe just something for us to be aware of, as it's not necessarily bad to consider alternatives in such cases
jfmcquade
left a comment
jfmcquade
left a comment
There was a problem hiding this comment.
Thanks Chris, nice to keep these changes as simple as possible, so I'm happy to move over to this PR.
A couple of comments, one inline and one in response to:
I don't see much downside to making the section highlight default (and only) behaviour - if opening a standalone template the nav bar will likely get covered anyway, and not aware of many other cases where user is expecting navigation to somewhere entirely outside of all sections. If that were the case we could always add an action to remove highlight.
There is an additional case we were considering that isn't just about navigating to a template outside of all sections, and that is navigating from a template belonging to one section directly to a template belonging to another without going via one of the top-level section pages (those pages linked to in the footer). E.g. If I navigate from template Section 1 > template A directly to Section 2 > template B, then Section 1 will still be highlighted in the footer even though I am supposed to be in Section 2. This leaves footer highlighting non-deterministic, as which button is highlighted can depend on how I navigated to the current template. This is why this method of persisting the highlighting was seen as a workaround that should maybe be disabled by default.
However this is an edge case, and I don't think is a big issue. In particular, as we don't actually have any formal concept or representation of "sections", it probably doesn't make sense to anticipate that at this stage and we should probably just handle that behaviour when that structure has some actual meaning.
| effect(() => { | ||
| // When user navigates to a section matching the nav button target | ||
| // set as the current highlighted section | ||
| const templateName = this.templateMetaService.templateName(); | ||
| const targetTemplates = this.targetTemplates(); | ||
| const updatedSection = targetTemplates.findIndex((name) => name === templateName); | ||
| if (updatedSection > -1) { | ||
| this.activeLinkIndex.set(updatedSection); | ||
| } | ||
| }); |
There was a problem hiding this comment.
@chrismclarke – as you highlighted in #3295 (comment) (and I agree), this feels like an overzealous application of the general rule to avoid updating signals in effects where possible. Obviously Angular has now made this behaviour the default, but it is still a pattern that is discouraged as far as I can tell, but in some cases it's clearly much better than the alternatives. Possibly worth updating our styleguide? Or maybe just something for us to be aware of, as it's not necessarily bad to consider alternatives in such cases
|
|
||
| /** Name of current template provide by route param */ | ||
| private templateName = computed<string | undefined>(() => this.snapshot().params.templateName); | ||
| public templateName = computed<string | undefined>(() => this.snapshot().params.templateName); |
There was a problem hiding this comment.
nit(non-blocking): As you point out in #3296 (comment), this is simpler than moving the property to the nav service. However my feeling was that the nav service is the correct place for it – the template metadata service is specifically for handling metadata on the current template, so if this property is going to be consumed in various places it might make sense to move it to the template nav service. But I'm not overly bothered by it remaining here
There was a problem hiding this comment.
I'd say given that both the nav service and the meta service will likely want to know the name of the template, it feels more natural to me that the nav service reads it as template meta than the meta service reads as something to do with nav (metadata is a lower-level service, so easier to avoid circular deps if keeping there).
But really no harm importing the meta service and including in the nav if useful as a method (e.g getCurrentTemplateName ) that returns
There was a problem hiding this comment.
@chrismclarke – as you highlighted in #3295 (comment) (and I agree), this feels like an overzealous application of the general rule to avoid updating signals in effects where possible. Obviously Angular has now made this behaviour the default, but it is still a pattern that is discouraged as far as I can tell, but in some cases it's clearly much better than the alternatives. Possibly worth updating our styleguide? Or maybe just something for us to be aware of, as it's not necessarily bad to consider alternatives in such cases
Yeah it's still pretty explicit in the docs (despite no longer requiring the allowSignalWrites param)
I think maybe we could just minimally update the style guide to be less forceful, notably allowing for effects to update signals following async function (as there's no async computed), or in cases where the computed logic would be significantly more complex. Would also be useful to remove the allowSignalWrites legacy code that is still around in a few components
2 participants
PR Checklist
Description
Alt implementation to #3296
Makes template section persist default (and only) behaviour
Dev Notes
I don't see much downside to making the section highlight default (and only) behaviour - if opening a standalone template the nav bar will likely get covered anyway, and not aware of many other cases where user is expecting navigation to somewhere entirely outside of all sections. If that were the case we could always add an action to remove highlight.
Git Issues
Closes #
Screenshots/Videos
(screen recording extension currently broken)