Skip to content

[4.0] com_media toolbar buttons#27251

Merged
wilsonge merged 19 commits intojoomla:4.0-devfrom
brianteeman:com_media
Jan 7, 2020
Merged

[4.0] com_media toolbar buttons#27251
wilsonge merged 19 commits intojoomla:4.0-devfrom
brianteeman:com_media

Conversation

@brianteeman
Copy link
Copy Markdown
Contributor

@brianteeman brianteeman commented Dec 13, 2019

PR for #27148

Makes sure that a button is a type=button
aria-hidden on the icon
Correct cursor on hover

@brianteeman
[4.0] com_media toolbar buttons
5b27520 
PR for joomla#27148

Makes sure that a button is a type=button
aria-hidden on the icon
Correct cursor on hover
@Quy Quy mentioned this pull request Dec 13, 2019
Closed
@Quy
Copy link
Copy Markdown
Contributor

Quy commented Dec 13, 2019

I have tested this item ✅ successfully on 5b27520

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27251.

1 similar comment
@ChristineWk
Copy link
Copy Markdown

ChristineWk commented Dec 13, 2019

I have tested this item ✅ successfully on 5b27520

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27251.

@Quy Quy removed the PR-4.0-dev label Dec 13, 2019
@Quy
Copy link
Copy Markdown
Contributor

Quy commented Dec 13, 2019

RTC

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27251.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 13, 2019
@Quy
Copy link
Copy Markdown
Contributor

Quy commented Dec 30, 2019

@HLeithner Can you please merge as checks constantly failed? Thanks.

@HLeithner
Copy link
Copy Markdown
Member

HLeithner commented Dec 30, 2019

The test has to fail because this pr changes the markup and the test can't press the button anylonger...

So whats the reason to change this from span to button? and shouldn't the joomla-toolbar-button simulate button @ggppdk or what do I miss?

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Dec 30, 2019 •
edited
Loading

This PR changes the markup to be consistent with other toolbar buttons in order to display the hover cursor as a hand and not an arrow.

@HLeithner
Copy link
Copy Markdown
Member

HLeithner commented Dec 30, 2019

Then the test has to be adapted... shouldn't be too hard but don't have time to look at it sorry maybe @Hackwar can have a look?

@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Dec 30, 2019

These failures can't be related to any changes here


[Selenium browser Logs]
--
666 | 00:24:47.537 SEVERE - http://localhost/test-install/media/layouts/js/joomla/html/batch/batch-language.min.js?6d89747421e0b9c3cc781287dd5b01dd 0:391 Uncaught TypeError: Cannot read property 'addEventListener' of undefined
667 | 00:24:48.192 SEVERE - http://localhost/test-install/media/layouts/js/joomla/html/batch/batch-language.min.js?6d89747421e0b9c3cc781287dd5b01dd 0:391 Uncaught TypeError: Cannot read property 'addEventListener' of undefined
668 | 00:24:53.810 SEVERE - http://localhost/test-install/media/layouts/js/joomla/html/batch/batch-language.min.js?6d89747421e0b9c3cc781287dd5b01dd 0:391 Uncaught TypeError: Cannot read property 'addEventListener' of undefined

@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Dec 30, 2019

Hopefully the tests are now updated correctly and will pass

@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Dec 31, 2019

My attempt at guessing the fix to the tests failed. @puneet0191 @Hackwar any help greatly appreciated. PS it does seem odd that the test named uploading files seems to be all about deleting files

@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Jan 4, 2020

I can confirm the tests is replicating real life. If I click on the delete toolbar item and then click on delete again in the modal that appears. The item is not actually deleted.

@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Jan 4, 2020

my bad - you are right - this vue js is beyond me :(

@brianteeman
Revert "update tests"
a3b9fcc 
This reverts commit 2480827.
@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Jan 4, 2020

The other buttons still work with this change and I just don't see the difference

@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Jan 4, 2020

The act of clicking the delete toolbar button is de-selecting the items. Not 100% sure why but I guess it's related to the active click area (or similar)

@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Jan 5, 2020

Hopefully it will pass the tests now

@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Jan 5, 2020

I think you still need the selector change too in the tests

@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Jan 5, 2020

@wilsonge it's still failing but with the usual timeout error - its not even getting as far as the relevant test. Dam these flaky tests are annoying

@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Jan 5, 2020

Woohoo - tests all pass now

@wilsonge wilsonge merged commit 22ab629 into joomla:4.0-dev Jan 7, 2020
@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Jan 7, 2020

Yay! Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 7, 2020
@wilsonge wilsonge added this to the Joomla 4.0 milestone Jan 7, 2020
@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Jan 7, 2020

thanks - got their in the end

@brianteeman brianteeman deleted the com_media branch January 7, 2020 20:04
@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Jan 8, 2020

got their

What have you done with the real Brian :P

@brianteeman
Copy link
Copy Markdown
Contributor Author

brianteeman commented Jan 8, 2020

he is teaching me to spell

brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Feb 4, 2020
@brianteeman
[4.0] com_media toolbar buttons (joomla#27251)
10df2eb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wilsonge wilsonge wilsonge left review comments

Assignees

No one assigned

Labels

Unit/System Tests

Projects

None yet

Milestone

Joomla 4.0

Development

Successfully merging this pull request may close these issues.

7 participants

@brianteeman @Quy @ChristineWk @HLeithner @wilsonge @joomla-cms-bot @infograf768