[Fix issue] Closes #309 - moved the undo banner below reset button#312
[Fix issue] Closes #309 - moved the undo banner below reset button#312renji18 wants to merge 3 commits into
Conversation
|
Hi @rugk, thanks for your help, I was able to successfully link this PR with the issue, thanks again. Please review the code and let me know if this good to be merged or not. |
|
Great, note you do not need to create a new PR when you make changes, just edit your previous PR or push into the same branch and it will end up in your PR. For furture reference, this is the old PR: #311 Linking can also be done manually at the right site AFAIK even for contributors. |
rugk
changed the title
rugk
left a comment
rugk
left a comment
There was a problem hiding this comment.
So while that solves the problem, we have a one big technical problem with this solution…
| <hr /><br /> | ||
| <button data-i18n="__MSG_optionsResetButton__" type="button" id="resetButton">Reset all settings to defaults</button> | ||
| </div> | ||
| <div id="messageSuccess" aria-label="success message" data-i18n data-i18n-aria-label="__MSG_ariaMessageSuccess__" class="message-box success invisible fade-hide success-undo-message-box"> |
There was a problem hiding this comment.
Hmm, good idea in theory, but moving all success messages there is likely not a good idea.
The thing is, you actually do not want to show all success messages here, because other modules could show other success messages and they should not be at the bottom. And if you just move the HTML element with id #messageSuccess that is just the the default container for success messages, but good news: the MessageHandler module can have more such arbitrary message locations/HTML elements.
That is why I've opened TinyWebEx/AutomaticSettings#28 and one would likely have to adjust that as well and then use the library here. (You can do so, it is a git submodule, online guides should tell you how to work with them…) See there for more details.
There was a problem hiding this comment.
So basically, I have to place the HTML I changed to the previous location, create a new id which displays the undo message, and then place a new HTML that only displays the undo message at the bottom. Did I get it right?
There was a problem hiding this comment.
Yeah that's it! The more or less tricky part is the fact that likely the AutomaticSettings library (module) needs to be adjusted to work with that message box instead of the success one - of course keep in mind it should at best have no breaking changes for other add-ons using the library, so that should be optional.
https://github.com/TinyWebEx/PermissionRequest does something similar/is used in the same way, as it also can register for arbitrary message boxes.
2 participants
Fixed issue which Closes #309