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

feat: Add Link component back #411

Merged
merged 33 commits into from
Feb 20, 2024
Merged

feat: Add Link component back #411

merged 33 commits into from
Feb 20, 2024

Conversation

tigerros
Copy link

@tigerros tigerros commented Dec 4, 2023

Features

  • Add Link, which works like Dioxus router's Link. Example code:

    rsx! {
        Link { to: Route::Foo {}, label { "Route default (no tooltip)" } }
        Link { to: Route::Bar {}, tooltip: AnchorTooltip::Custom("Bar".to_string()), label { "Custom tooltip" } }
        Link { to: "https://github.com/marc2332/freya", label { "URL default (URL value tooltip)" } }
        Link { to: "https://github.com/marc2332/freya", tooltip: AnchorTooltip::None, label { "URL no tooltip" } }
    }
  • Optimize ExternalLink a bit.

Notes

  • This makes ExternalLink obsolete, except for theming.
  • Link might be a better name.
  • This doesn't add any styles or themes. I think components should add the minimum amount of style. They can be wrapped in a rect to add shadows, change color, etc., but it's impossible to remove styles. As for the themes, I just didn't bother adding them, which is why this is a draft.

@tigerros tigerros changed the title enhancement: Add Anchor component + minor additions enhancement: Add Anchor component Dec 4, 2023
@tigerros tigerros changed the title enhancement: Add Anchor component feat: Add Anchor component Dec 4, 2023
@tigerros tigerros marked this pull request as draft December 4, 2023 05:16
Copy link

codecov bot commented Dec 4, 2023

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (8b9f961) 54.22% compared to head (8f14b66) 54.28%.
Report is 1 commits behind head on main.

❗ Current head 8f14b66 differs from pull request most recent head 0e0b66f. Consider uploading reports for the commit 0e0b66f to get more accurate results

Files Patch % Lines
crates/components/src/link.rs 0.00% 23 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #411      +/-   ##
==========================================
+ Coverage   54.22%   54.28%   +0.05%     
==========================================
  Files         154      154              
  Lines       13732    13717      -15     
==========================================
  Hits         7446     7446              
+ Misses       6286     6271      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marc2332 marc2332 added the enhancement 🔥 New feature or request label Dec 4, 2023
@marc2332
Copy link
Owner

marc2332 commented Dec 4, 2023

I actually had a Link component some time ago, but the problem is that it didn't work well because Freya doesn't have events bubbling yet, I am waiting for the Dioxus 0.5 release. Once that is done, we should definitely merge this! I would call the component Link instead of Anchor, yes.

@marc2332 marc2332 self-requested a review December 4, 2023 14:47
@tigerros
Copy link
Author

tigerros commented Dec 4, 2023

I actually had a Link component some time ago, but the problem is that it didn't work well because Freya doesn't have events bubbling

We could add a label string prop like in ExternalLink instead of children then. Alternatively, add a warning about this issue.

@tigerros
Copy link
Author

tigerros commented Dec 4, 2023

New things:

  • Renamed Anchor to Link.
  • Added the event propagation warning.
  • Removed ExternalLink and added theming from it to Link.
  • Migrated all references of ExternalLink to Link.

@marc2332 marc2332 changed the title feat: Add Anchor component feat: Add Link component back Dec 16, 2023
@tigerros tigerros mentioned this pull request Dec 31, 2023
@tigerros tigerros marked this pull request as ready for review December 31, 2023 23:12
@tigerros
Copy link
Author

tigerros commented Dec 31, 2023

New changes:

  • Added ExternalLink back for situations without dioxus-router.
  • Made the example better and it actually works (I didn't use dioxus-router previously).
  • Updated, fixed, polished.

@marc2332
Copy link
Owner

Amazing work @tigerros ! I just resolved all the conflicts and cleaned some code, this LGTM. I would deprecate ExternalLink in this same PR, thoughts?

@marc2332 marc2332 removed the blocked label Feb 12, 2024
@tigerros
Copy link
Author

Amazing work @tigerros ! I just resolved all the conflicts and cleaned some code, this LGTM. I would deprecate ExternalLink in this same PR, thoughts?

I don't think ExternalLink should be deprecated because Link only works with Dioxus's router and they do the same thing with external routes, with Link having some overhead.

@marc2332
Copy link
Owner

Amazing work @tigerros ! I just resolved all the conflicts and cleaned some code, this LGTM. I would deprecate ExternalLink in this same PR, thoughts?

I don't think ExternalLink should be deprecated because Link only works with Dioxus's router and they do the same thing with external routes, with Link having some overhead.

I can try to make Link work when dioxus-router is not being used, it shouldn't be that hard

@tigerros
Copy link
Author

Amazing work @tigerros ! I just resolved all the conflicts and cleaned some code, this LGTM. I would deprecate ExternalLink in this same PR, thoughts?

I don't think ExternalLink should be deprecated because Link only works with Dioxus's router and they do the same thing with external routes, with Link having some overhead.

I can try to make Link work when dioxus-router is not being used, it shouldn't be that hard

Did something change with hooks? I thought you had to call them every time so you can't remove use_navigator.

@marc2332
Copy link
Owner

Amazing work @tigerros ! I just resolved all the conflicts and cleaned some code, this LGTM. I would deprecate ExternalLink in this same PR, thoughts?

I don't think ExternalLink should be deprecated because Link only works with Dioxus's router and they do the same thing with external routes, with Link having some overhead.

I can try to make Link work when dioxus-router is not being used, it shouldn't be that hard

Did something change with hooks? I thought you had to call them every time so you can't remove use_navigator.

Nothing changed, but I can get the navigator myself in L138 instead of using the hook:

let router = try_consume_context::<RouterContext>()
            .expect("Must be called in a descendant of a Router component");

router.push(to.clone());

@marc2332
Copy link
Owner

Just found a smalllllllllll bug with the events system in Freya, will open a PR later, after that, I will push some final touches and this PR will be ready to go

@tigerros tigerros mentioned this pull request Feb 18, 2024
@marc2332
Copy link
Owner

works!

@marc2332
Copy link
Owner

marc2332 commented Feb 19, 2024

I'll add some unit tests before merging

@marc2332 marc2332 merged commit 84e108d into marc2332:main Feb 20, 2024
2 of 3 checks passed
@marc2332
Copy link
Owner

Nice work @tigerros !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🔥 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants