Skip to content
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

Angular navigation #38

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Angular navigation #38

wants to merge 4 commits into from

Conversation

Cybertron01Z
Copy link
Collaborator

No description provided.

@tispBe tispBe assigned tispBe and unassigned tispBe Jul 30, 2024
@tispBe tispBe self-requested a review July 30, 2024 06:45
Copy link
Collaborator

@tispBe tispBe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool article! already looking forward to the next one ;)
didn't know some of the router specifics you mention in the article.

There are only a few rephrasings, typo or clarifications needed. I feel like the start of the article could be improved a bit. To me the actual article only starts with the section "the new concept" where you introduce the actual requirements and problems that you faced. The previous sections to me are a bit out of context and might need further clarifications or context.

For the section titles, I would probably use the requirements as section title instead of asking questions in the title. You already did this with the title: Navigate back to hub pages. This could further improve the structure.

Apart from that, all good 👍 Well done!


# TLDR;
As with almost everything, using the browser standards is resulting in the best and easiest implementation for our application routing.
Previously we managed our own navigation page stack in memory, but it got out of sync when the native browser back/forward feature were used.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Previously we managed our own navigation page stack in memory, but it got out of sync when the native browser back/forward feature were used.
Previously we managed our own navigation page stack in memory, but it got out of sync when the native browser back/forward feature was used.

# What was our goal
We have already refactored our navigation concept twice.
Both times we have mainly used forward navigations.
Yes also for actions that should have been back navigations.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Yes also for actions that should have been back navigations.
Yes, also for actions that should have been back navigations.


**Disclaimer: This is a general concept, but the code snippets and some issue we solved were Angular specific.**

# TLDR;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intro of this article is a bit confusing IMO. When reading it for the first time, I feel like i missed out on part 1 of a 2 part series. You dive right in with

I would probably add some information on what you are building roughly, what this article is about, what problem you are facing

Moving to `location.back()` and `location.go(-x)` helped us solve this issue and made the implementation a lot more streamlined.

# What was our goal
We have already refactored our navigation concept twice.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, you tried to implement an on page back navigation feature so that the user can click to go to the previous page instead of using the browser back navigation buttons, right?
If so, then I would probably start this section with explaining what you were actually building. Otherwise, it's hard to follow.

# What was our goal
We have already refactored our navigation concept twice.
Both times we have mainly used forward navigations.
Yes also for actions that should have been back navigations.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like what?

drafts/angular-navigation-concept.md Show resolved Hide resolved
drafts/angular-navigation-concept.md Show resolved Hide resolved
drafts/angular-navigation-concept.md Show resolved Hide resolved
But we get stuck at the first `/a`.
The reason for this is, it is the same URL, and our config says we should ignore those cases.
Meaning, the guards are not called anymore and only `NavigationSkipped` is dispatched.
So far I have not found a solution to this problem.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you can add a shoutout to comment this article if anyone has an idea how to resolve it.

drafts/angular-navigation-concept.md Show resolved Hide resolved
@tispBe
Copy link
Collaborator

tispBe commented Aug 28, 2024

@Cybertron01Z what is the state of the article? shall I review it again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants