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

Prevents the row from being selected when clicked on an i-tag #1480

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

mko-sci
Copy link
Contributor

@mko-sci mko-sci commented Nov 19, 2024

Additional Notes

  • This PR fixes or works on following ticket(s): SIRI-11094

Checklist

  • Code change has been tested and works locally
  • Code was formatted via IntelliJ and follows SonarLint & best practices

@mko-sci mko-sci added 🐛 Bugfix Contains only a small fix for an existing bug 🧬 Enhancement Contains new features 👶🏻 Trivial Easy to review labels Nov 19, 2024
Comment on lines 33 to 36
if (event.target.tagName.toLowerCase() !== 'input' &&
event.target.tagName.toLowerCase() !== 'a' &&
event.target.tagName.toLowerCase() !== 'i' &&
event.target.tagName.toLowerCase() !== 'button') {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to maintain a whitelist or use a marker class that possible click-targets get assigned?

Copy link
Member

Choose a reason for hiding this comment

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

Also, it would be nice to know why particularly <i> tags in particular are supposed to be exempt? Why not <b> tags? Or <span> tags? To me, the entire PR lacks justification. (Not saying that it is not justified, but there is no reasoning.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Icons are often wrapped in <i>-tags. If you add an <a> which has an <i> inside and you manage to click directly on the <i> (or the icon) you would select the row. In this case however the intention was to click on the <a>-button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it be better to maintain a whitelist or use a marker class that possible click-targets get assigned?

I see what you mean. I will look into it if I have spare time if thats ok for you.

Copy link
Member

Choose a reason for hiding this comment

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

Icons are often wrapped in <i>-tags. If you add an <a> which has an <i> inside and you manage to click directly on the <i> (or the icon) you would select the row. In this case however the intention was to click on the <a>-button.

In case of FA, the <i> tag only serves as a marker and is replaced by a <svg> tag if i am not wrong.

@mko-sci mko-sci merged commit 938daa3 into develop Nov 20, 2024
6 checks passed
@mko-sci mko-sci deleted the feature/mko/OX-11094 branch November 20, 2024 13:30
@mko-sci
Copy link
Contributor Author

mko-sci commented Nov 20, 2024

Merged because This PR just extends the current mechanism.

Obvoiusly we need to rework the mechansim but there is currently no time for that since we are close the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bugfix Contains only a small fix for an existing bug 🧬 Enhancement Contains new features 👶🏻 Trivial Easy to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants