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.

Sign up for GitHub

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

Jump to bottom

Please review PR 454 (Uri.Compare, routes with #fragments) #455

Closed
dmitry-pavlov opened this issue Oct 17, 2023 · 5 comments · Fixed by #484
Closed

Please review PR 454 (Uri.Compare, routes with #fragments) #455

dmitry-pavlov opened this issue Oct 17, 2023 · 5 comments · Fixed by #484

Comments

@dmitry-pavlov
Copy link
Contributor

@mrpmorris please review #454 once you have time and consider for merging. TIA!
@mrpmorris
Copy link
Owner

Sorry for the delayed response, life got in the way!

Could put the details of the problem into the ticket, please? I don't know what is wrong with the current code.

Could you please also add one or more unit tests?

@dmitry-pavlov
Copy link
Contributor Author

dmitry-pavlov commented Oct 20, 2023 •
edited
Loading

I added that to handle routes Uri links as Uri links rather than as strings. Reason was the links with #fragments if I remember right.

As for the unit tests, sure, I will have a look what I can add. Perhaps, this weekend. Thanks!

@dmitry-pavlov dmitry-pavlov changed the title Please review PR 454 Please review PR 454 (Uri.Compare, links with #fragments) Oct 20, 2023
@dmitry-pavlov dmitry-pavlov changed the title Please review PR 454 (Uri.Compare, links with #fragments) Please review PR 454 (Uri.Compare, routes with #fragments) Oct 20, 2023
@mrpmorris
Copy link
Owner

Thank you, I appreciate your contribution and your patience!

@dmitry-pavlov
Copy link
Contributor Author

Hey @mrpmorris Sorry for delay. I got stuck figuring out what kind of test it could be... Any ideas are welcome.

Let me put some light on the reason why I needed that. Maybe it would be enough to merge this PR.

Blazor can't handle routes with fragments out of the box https://chrissainty.com/fragment-routing-with-blazor/

Saying fragments, I mean those URI Fragment part of anchor links to jump to a specific part of a page like this below:

<a href="#anchor-name">Jump to the part of the page with the “anchor-name” id </a>

In the app I am working on I have a page with tabs, which are handled by Blazor and use Fluxor.Blazor.Web.Middlewares.Routing.GoAction and routing middleware to navigate between tabs, and identify the which NavLink is active at the moment (to highlight on UI). In addition to that I have on-page navigation with "fragments" pointing to some element IDs on the page. I am trying to reduce using NavigationManager directly and instead - use routing middleware to handle in app navigation. So all navigation (without force page reloading and with it (when needed) is built on top of Fluxor.Blazor.Web.Middlewares.Routing classes.

To check if the route is the same in HandleGoActionAsync you just strings from NavigationManager.Uri and let it be navigated if the strings are different. For links with URI fragment it could a bit be more sophisticated if UrlComparer would sompare URIs rather than strings.

Please let me know what sort of test I could add for this PR, or feel free to merge the PR. Thanks!

@dmitry-pavlov dmitry-pavlov mentioned this issue Dec 24, 2023
Closed
@mrpmorris
Copy link
Owner

I don't know what unit tests you can add. I usually look at the unit tests to understand the purpose of the code.

I'm putting together V6 at the moment, don't miss out :)

@dmitry-pavlov dmitry-pavlov mentioned this issue Mar 22, 2024
Merged
dmitry-pavlov added a commit to dmitry-pavlov/Fluxor that referenced this issue Mar 22, 2024
@dmitry-pavlov
Add a note to Releases.md about mrpmorris#455
2fc120a
dmitry-pavlov added a commit to dmitry-pavlov/Fluxor that referenced this issue Mar 22, 2024
@dmitry-pavlov @mrpmorris @dependabot
Sync with the latest changes (#1)
419af40 
* Remove support for obsolete versions of .NET (mrpmorris#385)

* Change IDisposable to IAsyncDisposable (Fixes mrpmorris#324)

* ReduxDevTools should not call UseRouting (Fixes mrpmorris#360) (mrpmorris#388)

* Remove UseRouting from UseReduceDevTools

* Allow action filtering on redux dev tools (fixes mrpmorris#383) (mrpmorris#389)

* Support action filtering in Redux Dev Tools

* Add unit tests

* Remove Newtonsoft.Json (Fixes mrpmorris#386) (mrpmorris#392)

* Remove dependency on Newtonsoft.Json

* Remove support for obsolete versions of .NET (mrpmorris#385)

* Change IDisposable to IAsyncDisposable (Fixes mrpmorris#324)

* ReduxDevTools should not call UseRouting (Fixes mrpmorris#360) (mrpmorris#388)

* Remove UseRouting from UseReduceDevTools

* Allow action filtering on redux dev tools (fixes mrpmorris#383) (mrpmorris#389)

* Support action filtering in Redux Dev Tools

* Add unit tests

* Remove Newtonsoft.Json (Fixes mrpmorris#386) (mrpmorris#392)

* Remove dependency on Newtonsoft.Json

* Allow action filtering on redux dev tools (fixes mrpmorris#383) (mrpmorris#389)

* Support action filtering in Redux Dev Tools

* Add unit tests

* Bump Swashbuckle.AspNetCore.SwaggerUI

Bumps [Swashbuckle.AspNetCore.SwaggerUI](https://github.com/domaindrivendev/Swashbuckle.AspNetCore) from 6.3.0 to 6.4.0.
- [Release notes](https://github.com/domaindrivendev/Swashbuckle.AspNetCore/releases)
- [Commits](domaindrivendev/Swashbuckle.AspNetCore@v6.3.0...v6.4.0)

---
updated-dependencies:
- dependency-name: Swashbuckle.AspNetCore.SwaggerUI
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

* Deadlock mrpmorris#474 (mrpmorris#476)

* Add lock conflict test
* Alternate locking approach
* Update .net versions

* Eliminate deadlock when executing an action from IDispatcher.ActionDispatched on a different thread mrpmorris#474 (mrpmorris#477)

* Update SourceLink
* Fix flaky tests in ThrottledInvoker
* Execute feature callbacks outside lock()

* Have ThrottledInvoker re-enter if the timer executes too soon.

* Have test ensure the execution window is within (ThrottleWindowMs) and (ThrottleWindowMs* 2)

* Change parameter name from action to throttledAction in test.

* Don't test for too long, only too short.

* Task.Yield to ensure fire-and-forget action logging has time to complete

* Update throttling tests

* Update throttling test comments

* Rewrite ThrottledInvoker to not use Timers

* Source formatting

* Make throttled invoker disposable

* Call ThrottledInvoker.Dispose  from Blazor libraries

* ThrottledInvoke.Dispose tests

* Change style of unit test

* Remove race condition from test

* Change test timings

* Spelling error

* Use IAsyncLifetime instead of using Task.Wait()

* Code tidy

* Remove NCrunch file

* Ignore *ncrunch*

* Use TimeSpan instead of int MS

* 6.0-Beta1

* Fix errors in tests detected by NCrunch.

* Unit tests for navigation fragments

* Rename UrlComparer.SameAs to UrlComparer.AreEqual

* Delete TestNavigationManager

* Add a note to Releases.md about mrpmorris#455

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Peter Morris <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #455 dmitry-pavlov/Fluxor
2 participants
@dmitry-pavlov @mrpmorris