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 replace prop to KRouterLink #152

Merged
merged 2 commits into from
Feb 12, 2021

Conversation

sairina
Copy link
Contributor

@sairina sairina commented Feb 10, 2021

Summary

Added replace prop to KRouterLink by making replace accessible within <router-link> component.

Addresses #85

@sairina sairina marked this pull request as ready for review February 11, 2021 22:20
@nucleogenesis
Copy link
Member

Hey @sairina - we'll want to have the replace prop defined on the KRouterLink component as well - then the :replace="replace" in the template. I wasn't clear in the original issue, but the desire is to give the option to the implementer whether or not they want to replace.

I think that since the default is false on <router-link> we should make it the default in our KRouterLink as well

@sairina
Copy link
Contributor Author

sairina commented Feb 12, 2021

Thanks, for the clarification @nucleogenesis! Does this now work as intended?

Showing use of replace prop:

replace="true"

Expected behavior: the route to /test doesn't get added as a new history object; it replaces the current one (/info), so clicking the browser's back button takes it back to /content, and clicking the forward button takes it to /test, as /info has been replaced by /test.

** Flow: **

  1. To - Channels (/content)
  2. To - Info (/info)
  3. To - Test (/test)
  4. Back Arrow - Channels (/content)
  5. Forward Arrow - Test (/test)

2021-02-11 20 56 14

replace="false"

Expected behavior: the route to /test does get added as a new history object, so clicking the browser's back button takes it back to /info.

** Flow: **

  1. To - Channels (/content)
  2. To - Info (/info)
  3. To - Test (/test)
  4. Back Arrow - Info (/info)
  5. Forward Arrow - Test (/test)

2021-02-11 20 56 09

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

LGTM!

@sairina sairina merged commit a1c8e18 into learningequality:v0.2.x Feb 12, 2021
@sairina sairina linked an issue Feb 12, 2021 that may be closed by this pull request
Comment on lines +65 to +68
replace: {
type: Boolean,
required: false,
},
Copy link
Contributor

@indirectlylit indirectlylit Feb 14, 2021

Choose a reason for hiding this comment

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

To avoid ambiguity and undefined input values, component props should either be required: true or provide a default value.

In this case, the prop should be:

      replace: {
        type: Boolean,
        default: false,
      }

This ought to be caught by a linting rule. Can follow up separately on why it wasn't:
https://eslint.vuejs.org/rules/require-default-prop.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I fixed it in a separate branch as a PR #162. Here. Not sure why that wasn't caught?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I missed that - good eye @indirectlylit !

@sairina sairina deleted the krouterlink-replace branch February 15, 2021 19:40
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.

KRouterLink - Add replace prop
3 participants