-
Notifications
You must be signed in to change notification settings - Fork 54
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: added router-ignore attribute to ignore the links (#325) #421
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, @miladkdz! You are making Vaadin Router even more awesome, bit by bit.
Please fix the blocking review comments about the vaadin-router-ignore
router-ignore
typo.
I've also added some style-related comments, but they are informational and you do not have to address them. The PR will get merged anyway.
Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladkdz)
demo/vaadin-router-getting-started-demos.html, line 249 at r1 (raw file):
<h3>Ignore attribute</h3>
STYLE
Would this heading catch the eye of users looking for this feature? Would this be one of the key words users would search for when looking for this feature on the demo page?
Judging by the terms people used to request for this feature in #325 and #417 (and also in internal DX tests a while ago), ignore attribute
is probably not the best heading for this feature.
How about something that's more likely to match the key words users use to describe the feature?
- External links
- Preventing Router from some links
- Excluded links
What do you think?
demo/vaadin-router-getting-started-demos.html, line 250 at r1 (raw file):
You can tell Vaadin Router to ignore a link ...
STYLE
Let's keep the text in one style, and avoid guiding users to 'talk to Vaadin Router'.
In other demos on this page the text is either written in the passive tense (like in "When the base URL is set, only the links matching the base URL are handled by the router"), or has Router as an actor (like in "Vaadin Router supports cases when the app is deployed to a subpath"). Let's try to keep it similar for this one as well.
Another comment here:
At the moment this text focuses on how Router works (Router will ignore all links with the router-ignore
attribute). Can it be written in a way that focuses on the outcomes more important for the user? (When some <a>
links are external for the app, Vaadin Router can be configured to ignore them and let the browser handle these links in the default way).
demo/vaadin-router-getting-started-demos.html, line 257 at r1 (raw file):
<!-- not handled as indicated by router-ignore attribute -->
STYLE
Let's avoid too long lines and put the comment in a separate line above. What do you think?
demo/vaadin-router-getting-started-demos.html, line 272 at r1 (raw file):
</script> </template> </vaadin-demo-snippet>
Here is a good place to add another demo for bulk ignoring external URLs:
routes = [
// this may be the first route in the config
{
path: '/pattern/to/exclude/(.*)',
action: (ctx, commands) => {
window.location.pathname = ctx.pathname;
}
},
...
];
src/triggers/click.js, line 72 at r1 (raw file):
vaadin-router-ignore
router-ignore
test/triggers/click.spec.html, line 27 at r1 (raw file):
vaadin-router-ignore
router-ignore
test/triggers/click.spec.html, line 355 at r1 (raw file):
vaadin-router-ignore
router-ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
Tried to go by the checklist in the issue one by one but could not do this:
Could you help me on this one @vlukashov?
Fixes #325
This change is