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: added router-ignore attribute to ignore the links (#325) #421

Merged
merged 3 commits into from
Dec 18, 2019

Conversation

miladkdz
Copy link
Contributor

@miladkdz miladkdz commented Dec 13, 2019

Tried to go by the checklist in the issue one by one but could not do this:

In the same demo also mention how to exclude multiple URLs at once with a custom action

Could you help me on this one @vlukashov?

Fixes #325


This change is Reviewable

Copy link
Contributor

@vlukashov vlukashov left a 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

Copy link
Contributor

@vlukashov vlukashov left a 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: :shipit: complete! all files reviewed, all discussions resolved

@vlukashov vlukashov merged commit b5b80ad into vaadin:master Dec 18, 2019
vlukashov pushed a commit that referenced this pull request Jan 8, 2020
vlukashov pushed a commit that referenced this pull request Jan 8, 2020
Follow-up on #421, related to #325

fix linter issue
haijian-vaadin pushed a commit that referenced this pull request Jan 9, 2020
Follow-up on #421, related to #325

fix linter issue
@bsutton bsutton mentioned this pull request Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent router from some paths
2 participants