Add padding to play/restart/remove buttons and update pausePlayToggle… #30
Conversation
… to match button on textContent
|
|
||
| //for pause play | ||
| function PausePlayToggle(elem, id) { | ||
| if (elem.textContent == 'Pause') { |
There was a problem hiding this comment.
what's the benefit of using '.textContent' instead of '.innerHTML' here.
There was a problem hiding this comment.
The buttons (Play/Pause, Restart, Remove) have no padding between the icon images and the text. In order to add padding between the icon and the text on those buttons, I had to place spans around the text.
When placing spans around the Play/Pause text, it breaks the PausePlayToggle function because that function updates the functionality of the button based on the text content of the button. Originally that function matched based on the innerHTML. However, the innerHTML of that button then becomes "<span...>Play", which doesn't match 'Play'.
Matching on innerHTML means that adding the inner spans for padding causes the Play/Pause button to stop working since the function can no longer find it in the DOM. By matching based on text content instead, the function does not break and is also more scalable. You can update the HTML of the button and it will still work provided the text content remains 'Play' or 'Pause'.
There was a problem hiding this comment.
Please let me know if I'm not making sense in my explanation. It's kind of a weird thing to describe. Also, in this PR, the padding I added between the text and icons in the buttons is really small (2px), which is maybe why it wasn't so noticeable.
|
what's the benefit of this change? |
|
Sorry, labelling the PR as 'draft' meanwhile. |
2 participants
… to match button on textContent
To add padding around the button text, I had to place them in spans. For the PausePlayToggle function to continue working, I had to update it to match the buttons based on textContent rather than innerHTML.