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 'isExact' option to active link #520

Merged
merged 1 commit into from Sep 20, 2019

Conversation

maier49
Copy link
Contributor

@maier49 maier49 commented Sep 9, 2019

Type: feature

The following has been addressed in the PR:

Description:
Adds an isExact option to the ActiveLink properties that if true will only add active classes if the matched outlet's isExact function returns true.
Resolves #492

@maier49 maier49 requested a review from agubler Sep 11, 2019
@@ -8,6 +8,7 @@ import Router from './Router';

export interface ActiveLinkProperties extends LinkProperties {
activeClasses: SupportedClassName[];
isExact?: boolean;
Copy link
Member

@agubler agubler Sep 11, 2019

Choose a reason for hiding this comment

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

Is having an isExact property a breaking change by default? As if it is not passed then it will fall back to a partial match? Would it be better to have isPartial, with the default being false/undefined and therefore requiring an exact match.

Another thought, do you think there would ever be a need to want to match specific params partially? In dojo/site we partially match for a specific param depending on the link being rendered, if we just have a boolean option of isPartial or isExact it would actually consider every link active even though they are each for a specific param?

Copy link
Member

@agubler agubler Sep 11, 2019

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@maier49 maier49 Sep 13, 2019

Choose a reason for hiding this comment

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

I don't see how isExact is a breaking change. By default it's not specified, and if it's false the behavior is the same as what's current: https://github.com/dojo/framework/pull/520/files#diff-3e02e2cbd03b930052645ddb1510bc2dR62

Do you mean it's breaking if it's true, because you can't specify isExact on an ActiveLink currently so how would that break?

I can certainly look into adding something to match specific params. But, could that be separate from this? As I understand it all this change is doing is giving an option so that you can opt in to only count links as active if the context that would get passed to your outlet has isExact() returning true. If you don't specify the property everything behaves as it does now and this could coexist with any other changes to param matching.

Copy link
Member

@agubler agubler Sep 13, 2019

Choose a reason for hiding this comment

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

You're absolutely right, sorry I was thinking about matching params "exactly" for a route which is a different (but useful) change to this.

Copy link
Member

@agubler agubler left a comment

A query on the new property

@maier49 maier49 merged commit 61ca416 into dojo:master Sep 20, 2019
4 checks passed
@maier49 maier49 deleted the active-link-specify-exact branch Sep 20, 2019
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.

2 participants