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(ui/dashboards): prevent default on anchor tag #14434

Merged
merged 4 commits into from
Jul 24, 2019
Merged

Conversation

OfTheDelmer
Copy link
Contributor

Describe your proposed changes here.
Fixes an issue where clicking on a dashboard name from the dashboards index caused an incorrect redirect due to default behavior on the anchor tag.

@OfTheDelmer OfTheDelmer requested a review from a team July 24, 2019 15:13
@ghost ghost requested review from lukevmorris and removed request for a team July 24, 2019 15:13
@OfTheDelmer OfTheDelmer requested a review from chnn July 24, 2019 15:27
Copy link
Contributor

@chnn chnn left a comment

Choose a reason for hiding this comment

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

Sweet. I think there's other places we have this same problem (for example, opening the edit bucket overlay).

I think that everywhere we are using an anchor tag, we should be using the Link component from react-router instead.

@OfTheDelmer
Copy link
Contributor Author

@chnn yeah I noticed the resource card from @influxdata/clockface differs in that it doesn't use an anchor tag. It looks like none of the components using ResourceList.EditableName specify an hrefValue so it could probably be removed. However, only 3 of 8 components currently have the e.preventDefault(). I'll go ahead and add it there. Maybe after we update to 0.0.17 we can swap these with the @influxdata/clockface implementation.

@chnn
Copy link
Contributor

chnn commented Jul 24, 2019

@OfTheDelmer awesome. I do think we're using the hrefValue in a few places:

hrefValue={this.editBucketLink}

hrefValue={this.editVariablePath}

I tried real quick adding e.preventDefault to the EditableName component itself, but that didn't seem to work. We could also try replacing the a with a div or Link in that component so that we don't have to e.preventDefault() everywhere it's used.

Also found a duplicate EditableName component that can probably be deleted if the one place it is used is updated:

https://github.com/influxdata/influxdb/blob/master/ui/src/shared/components/EditableName.tsx

@OfTheDelmer
Copy link
Contributor Author

@chnn didn't realize the ResourceList.Name also had this hrefValue implementation, but since none of the ResourceList.EditableName are currently using it I'll just swap it out with an div and see how that goes.

@OfTheDelmer
Copy link
Contributor Author

I tried real quick adding e.preventDefault to the EditableName component itself, but that didn't seem to work.

@chnn I experienced something similar this morning when trying to test the initial prevent default fix, but now find that running yarn build instead of yarn start reveals this does seem to fix this issue.

@chnn
Copy link
Contributor

chnn commented Jul 24, 2019

Oo weird, I think @121watts was experiencing something similar yesterday.

Does the behavior exist when using the hrefValue with a Link? My hunch is that is the combination we should be using everywhere.

@OfTheDelmer OfTheDelmer requested a review from chnn July 24, 2019 17:48
@OfTheDelmer OfTheDelmer merged commit e7a92f5 into master Jul 24, 2019
@mark-rushakoff mark-rushakoff deleted the fix/dash-index branch April 16, 2020 20:27
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.

3 participants