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.

Sign up for GitHub

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 issue] Closes #309 - moved the undo banner below reset button #312

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+10 −8
Open

[Fix issue] Closes #309 - moved the undo banner below reset button #312

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb006e0
[Fix] error # 309 (moved the undo button below reset button)
renji18 Aug 19, 2023
7b3a117
[Fix] issue #309 (moved undo banner below reset button)
renji18 Aug 19, 2023
22ec62e
Fix issue #309
renji18 Aug 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
View file
Original file line number Diff line number Diff line change
@@ -16,6 +16,7 @@
- Metta Ong ([@ongspxm](https://github.com/ongspxm))
- Michael Corwin ([@mcorwin22](https://github.com/mcorwin22))
- Pradyumna Mahajan ([@pradyumnamahajan](https://github.com/pradyumnamahajan))
- Aadarsh Jha ([@renji18](https://github.com/renji18))

## Translators

2 changes: 1 addition & 1 deletion src/common/modules/CommonCss
Submodule CommonCss updated from bdd81c to 56a45a
15 changes: 8 additions & 7 deletions src/options/options.html
View file
Original file line number Diff line number Diff line change
@@ -29,13 +29,7 @@
</a>
<img class="icon-dismiss invisible" src="/common/img/close.svg" width="24" height="24" tabindex="0" data-i18n data-i18n-aria-label="__MSG_dismissIconDescription__"></span>
</div>
<div id="messageSuccess" aria-label="success message" data-i18n data-i18n-aria-label="__MSG_ariaMessageSuccess__" class="message-box success invisible fade-hide">
<span class="message-text">That worked!</span>
<a href="#">
<button type="button" class="message-action-button micro-button success invisible"></button>
</a>
<img class="icon-dismiss invisible" src="/common/img/close.svg" width="24" height="24" tabindex="0" data-i18n data-i18n-aria-label="__MSG_dismissIconDescription__"></span>
</div>

<div id="messageError" aria-label="error message" data-i18n data-i18n-aria-label="__MSG_ariaMessageError__" class="message-box error invisible fade-hide">
<span class="message-text">An error happened.</span>
<a href="#">
@@ -195,6 +189,13 @@ <h1 data-i18n="__MSG_titleAddonBehavior__">Addon behavior</h1>
<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">
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

@rugk rugk Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

<span class="message-text">That worked!</span>
<a href="#">
<button type="button" class="message-action-button micro-button success invisible"></button>
</a>
<img class="icon-dismiss invisible" src="/common/img/close.svg" width="24" height="24" tabindex="0" data-i18n data-i18n-aria-label="__MSG_dismissIconDescription__"></span>
</div>
</form>
<p class="helper-text">
<span data-i18n="__MSG_translatorCredit__" data-opt-i18n-keep-children>