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

add support for onClick link override and fix Bulma sample #393

Merged
merged 7 commits into from
Mar 8, 2025

Conversation

slightfoot
Copy link
Contributor

@slightfoot slightfoot commented Mar 1, 2025

Description

I noticed the Bulma CSS example tabs no longer saved their state. This got me looking and a classic issue was found. There was a link tag used within the li which contains the onClick handler.

This means the link will always reload the page and not let the outer onClick handler take place. It is still correct however to put a link inside the li tag as anchors will still action even if javascript is disabled, they also are followed by bots and screen-readers for the visually impared. So moving the onClick handler from the li to the a tag would be the correct fix.

This leads to the issue resolved here. The default events() function does not allow for preventing the default handler to execute. Which is correct behaviour as their is very little now apart from a tags that have the default event behaviour.

This can be resolved a few ways:

  1. onClick handler can return false (classic html 4)
  2. onClick can call preventDefault on the event given to the callback (html 5)

So this PR adds a onClick parameter to the a tag function builder that injects the event preventDefault action so the link does not action and cause a browser navigation event. If a Jaspr user does not want this behaviour that can still override the events parameter and use the events() generator.

https://github.com/slightfoot/jaspr/blob/9871addf7bfcc03eadf948d37ce3a05f4b61409f/packages/jaspr/lib/src/components/html/text.dart#L56

https://github.com/slightfoot/jaspr/blob/9871addf7bfcc03eadf948d37ce3a05f4b61409f/apps/jaspr_pad/samples/bulma/tabs.dart#L56

Warning

This is untested.

Type of Change

Ready Checklist

  • I've read the Contribution Guide.
  • In case this PR changes one of the core packages, I've modified the respective CHANGELOG.md file using
    the semantic_changelog format.
  • I updated/added relevant documentation (doc comments with ///).
  • I added myself to the AUTHORS file (optional, if you want to).

Sorry, something went wrong.

@slightfoot slightfoot requested a review from schultek as a code owner March 1, 2025 19:25
Copy link

docs-page bot commented Mar 1, 2025

To view this pull requests documentation preview, visit the following URL:

docs.page/schultek/jaspr~393

Documentation is deployed and generated using docs.page.

Copy link

github-actions bot commented Mar 1, 2025

Package Version Report

The following packages have been updated:
jaspr : 0.18.0 -> 0.18.1
jaspr_builder : 0.18.0 -> 0.18.1
jaspr_cli : 0.18.0 -> 0.18.1
jaspr_test : 0.18.0 -> 0.18.1

Copy link
Owner

@schultek schultek left a comment

Choose a reason for hiding this comment

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

Generally I like this very much, I had this same problem many times but never thought of just defaulting to doing preventDefault in a custom handler.

Though as you see in my comment below the solution cannot directly modify the text.dart file as it is generated.

@slightfoot
Copy link
Contributor Author

@schultek patches made, should be good now

schultek
schultek previously approved these changes Mar 8, 2025
Copy link
Owner

@schultek schultek left a comment

Choose a reason for hiding this comment

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

LGTM

@schultek schultek merged commit 6d081a5 into schultek:main Mar 8, 2025
3 checks passed
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.

None yet

2 participants