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

Fix KV Version History queryParams on the component LinkedBlock #12079

Merged
merged 5 commits into from Jul 14, 2021

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented Jul 14, 2021

The bug: Originally versions.hbs was calling the component ListItem (which passes params through to LinkedBlock) using an ember helper query-params. However, there appear to be some issues with this helper—see the issue here. (As an aside I could not find solid Ember documentation on this helper and suspect though we use it in other places in the app, it's not something we should continue to use). Because of this, the actual query param ?version=X was not be amended onto the params object that made its way here. Thus the transitionTo method was borked.

How I fixed it: The LinkedBlock component was previously setup to handle queryParams by passing in the object directly. We followed this pattern and it fixed the bug. Additionally, I added test coverage and documentation on LinkedBlock so it would be a little easier to understand how to use it.

Bug before to reproduce you have to click on the linkedblock. there is another nested link in the block that works. to hit it, just click on the edges of the block, away from the text.
broken

Fixed bug
fixed

@Monkeychip Monkeychip added the ui label Jul 14, 2021
@vercel vercel bot temporarily deployed to Preview – vault July 14, 2021 20:11 Inactive
@Monkeychip Monkeychip added the bug Used to indicate a potential bug label Jul 14, 2021
@Monkeychip Monkeychip added this to the 1.8 milestone Jul 14, 2021
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 14, 2021 20:17 Inactive
Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

👏 excellent work, thanks for the extra docs and tests!

@Monkeychip Monkeychip merged commit 0b92a96 into main Jul 14, 2021
@Monkeychip Monkeychip deleted the ui/kv-fix-version-link branch July 14, 2021 21:38
Monkeychip added a commit that referenced this pull request Jul 15, 2021
* fix the issue

* add test coverage

* add documentation to link-block

* add changelog

* modify for browserstack
Monkeychip added a commit that referenced this pull request Jul 21, 2021
…) (#12096)

* fix the issue

* add test coverage

* add documentation to link-block

* add changelog

* modify for browserstack
jartek pushed a commit to jartek/vault that referenced this pull request Sep 11, 2021
…icorp#12079)

* fix the issue

* add test coverage

* add documentation to link-block

* add changelog

* modify for browserstack
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants