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 click event bubbling in Table::onRowClick() #1655

Merged
merged 27 commits into from
Sep 7, 2022
Merged

Conversation

ibelar
Copy link
Contributor

@ibelar ibelar commented Aug 6, 2021

fix #1601

@ibelar ibelar added the RTM label Aug 6, 2021
src/Table.php Outdated Show resolved Hide resolved
@mvorisek mvorisek changed the title [feature] Allow to exclude selectors when using Table::onRowClick() Allow to exclude selectors when using Table::onRowClick() Oct 3, 2021
Copy link
Member

@mvorisek mvorisek left a comment

Choose a reason for hiding this comment

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

If I understand the issue, we need to prevent event bublibling from controls in table cells. This does not solve it, at least not automatically. Then I belive we do not even need the option to pass any selector to exlude (and for rare cases, the user can do it on it's own as the method is simple).

@mvorisek mvorisek removed the RTM label Oct 10, 2021
@mvorisek mvorisek force-pushed the fix/table-row-click branch 2 times, most recently from 0192317 to 057adc3 Compare August 31, 2022 21:12
@mkrecek234
Copy link
Contributor

mkrecek234 commented Sep 1, 2022

A thought to get this finalised: Shouldn't we add a class to all links, checkboxes or other clickable items that are added to a table row, just like you did it for Checkbox::CHECKBOX_COLUMN_CSS like atk4-table-column-clickable. This class we set automatically for classes in Atk4\Ui\Table\Column like you did for Checkboxes, but also for Link. We can then define function onRowClick($action, array $excludeSelector = ['atk4-table-column-clickable]). So developers have all flexibility.

Whatever you guys consider best to get this released ;-) Please comment and I can adjust the code.

@mkrecek234
Copy link
Contributor

mkrecek234 commented Sep 2, 2022

We should do a generalisation by a defined class, here atk4-norowclick.
The selector td:not(atk4-norowclick) does not work, it does not prevent the action be triggered as you can see in the grid.php demo.

@mkrecek234
Copy link
Contributor

mkrecek234 commented Sep 2, 2022

Fixed, working now as specified - please check demo and revert demo changes in grid.php before merge

@mkrecek234 mkrecek234 added the RTM label Sep 2, 2022
@mvorisek mvorisek removed the RTM label Sep 2, 2022
demos/collection/grid.php Outdated Show resolved Hide resolved
demos/collection/grid.php Outdated Show resolved Hide resolved
src/Table.php Outdated Show resolved Hide resolved
src/Table.php Outdated Show resolved Hide resolved
src/Table.php Outdated Show resolved Hide resolved
@mkrecek234 mkrecek234 changed the title Allow to exclude selectors when using Table::onRowClick() Exclude checkbox, links and classes marked atk4-norowclick from Table::onRowClick() Sep 2, 2022
@mvorisek
Copy link
Member

mvorisek commented Sep 2, 2022

@mkrecek234 PR looks very well made!

I just need to write the test.

Also, the coverage has decreased hugely. I am already on it. The problem is present since #1840. It seems there is some bug with https://github.com/sebastianbergmann/php-code-coverage itself when caching is enabled.

@mvorisek mvorisek changed the title Exclude checkbox, links and classes marked atk4-norowclick from Table::onRowClick() Fix click event bubling in Table::onRowClick() Sep 2, 2022
@mvorisek mvorisek changed the title Fix click event bubling in Table::onRowClick() Fix click event bubbling in Table::onRowClick() Sep 2, 2022
@mvorisek mvorisek merged commit bfbf427 into develop Sep 7, 2022
@mvorisek mvorisek deleted the fix/table-row-click branch September 7, 2022 18:58
@mvorisek
Copy link
Member

mvorisek commented Sep 7, 2022

Thank you @ibelar and @mkrecek234!

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.

Table->onRowClick() - Ignore click on checkboxes
3 participants