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: Fix links in Announcements #1934

Merged
merged 16 commits into from
Aug 11, 2022

Conversation

ozandogrultan
Copy link
Contributor

@ozandogrultan ozandogrultan commented Jul 19, 2022

Signed-off-by: Ozan Dogrultan ozan.dogrultan@deliveryhero.com

Fixes #1875

Summary of Changes

Previously, when there were links in the Announcement html content, they were overridden by the navigation of the Card component and when they were clicked, announcement list page was shown instead of the link itself.

The changes in the PR introduces the logic the stop bubbling up the click event to the Card component so that the link navigation works as expected.

Tests

No test change is needed.

Documentation

No documentation change is needed.

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does

Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
@feng-tao
Copy link
Member

cc @Golodhros

@ozandogrultan
Copy link
Contributor Author

@Golodhros could you review this please? 🙏🏼

Comment on lines 49 to 57
<div
className="card-copy"
role="button"
tabIndex={0}
onClick={stopEvent}
onKeyDown={stopEvent}
>
{copy}
</div>
Copy link
Member

Choose a reason for hiding this comment

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

This creation of a fake button and adding a stop propagation handler to it seems a bit fishy to me.
Could we achieve the same effect by using CSS pointer-events?
Check in https://css-tricks.com/almanac/properties/p/pointer-events/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the link!
I've tried using pointer-events on both child and parent elements, however click event is always propagated to the Card component which results in following the navigation..
Could you be more specific on how we can use pointer-events here?

Copy link
Member

Choose a reason for hiding this comment

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

It is something like this: https://stackoverflow.com/a/46707009

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried this suggestion as well, but it didn't work either.. but I might be doing something wrong, I don't have much experience with pointer-events

Another option in my opinion is that eventPropagation can be stopped on the SanitizedHtml level here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we stop the propagation in the "html_content" level it also works, but for me it's just another workaround and doesn't solve the problem fully

Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
@ozandogrultan ozandogrultan requested a review from a team as a code owner August 10, 2022 10:48
@boring-cyborg boring-cyborg bot added the area:databuilder From databuilder folder label Aug 10, 2022
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
@ozandogrultan
Copy link
Contributor Author

@Golodhros I've changed the fix strategy slightly with the latest changes. Now the only change is to allow onclick handlers to be customized in the sanitized announcement content, so stopping event propagation logic can be set if needed by the content which is passed. Can you please review again?

Copy link
Member

@Golodhros Golodhros left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
@Golodhros Golodhros merged commit 7d80884 into amundsen-io:main Aug 11, 2022
@ozandogrultan ozandogrultan deleted the fix/links-in-announcements branch August 16, 2022 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:databuilder From databuilder folder area:frontend From the Frontend folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Links in the Announcement html content don't work properly in homepage
3 participants