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

UI: Implementation of Toast Container #4059

Merged
merged 1 commit into from
Mar 15, 2022
Merged

UI: Implementation of Toast Container #4059

merged 1 commit into from
Mar 15, 2022

Conversation

iszmais
Copy link
Contributor

@iszmais iszmais commented Feb 18, 2022

Implementation of the given Concept of ILIAS Toast Containers
Depends on #4049 since its his implementation

@iszmais iszmais changed the title Feature/toast container implementation DRAFT: Feature/toast container implementation Feb 18, 2022
@mjansenDatabay mjansenDatabay added kitchen sink php Pull requests that update Php code improvement labels Feb 22, 2022
@mjansenDatabay mjansenDatabay changed the title DRAFT: Feature/toast container implementation UI: Implementation of Toast Container Feb 23, 2022
@iszmais
Copy link
Contributor Author

iszmais commented Mar 1, 2022

@klees @Amstutz

this is ready to be reviewed. Fell free to give feedback or merge.

Greetings,
@iszmais

@iszmais iszmais requested review from klees and Amstutz March 1, 2022 15:54
@Amstutz Amstutz self-assigned this Mar 2, 2022
@Amstutz
Copy link
Contributor

Amstutz commented Mar 2, 2022

Hi @iszmais

Thx a lot. I tested the example. IMO it all works as expected and is looking good. However, some details need some attention still:

Please change, or give some reasoning why you prefer not to:

  • Examples I: checked out the examples. Thx a lot for adding! However, They all render at once. This will make it hard to have good test cases for those. Can you provide a button for each, generating a new request adding some get param to the request which then only rendered the toast specified in the get Param? Something like a button with “Show Toast Example 1” or similar.

Please change:

  • Code Format: Put the js in withAdditionalOnLoadCode on two lines, to enhance the readability, see: https://github.com/ILIAS-eLearning/ILIAS/pull/4059/files#diff-d08c76b5cf5210b2e917f668ef5c1fdcbd87f3e3172e01c2685a616e511911e9R67
  • Accessibility: If I understand this correctly, the whole container should be tagged as “aria-live” region, see= https://developer.mozilla.org/de/docs/Web/Accessibility/ARIA/ARIA_Live_Regions . I would propose, aria-live="polite", which tells the screenreader, to only inform the user about the notification, as soon as he/she is done editing the current task.
  • Examples II: Please use existing Icons for the examples. This is important for me, since I often use the examples while fixing CSS issues. This works better, if the example looks as closely to the real use cases as possible. E.g. use the Mail Icon or similar.
  • Tests I: I did not find the Toasts Unit Test in the Branch. Did I miss something? Please add tests for every method on the public interface.
  • Tests II: Also add a test for making sure tests/UI/Client/Toast/toast.test.js is rendered by php as expected. We had the issue in the past, the the client side test was working, but on the basis of the html that was not the html being rendered by php anymore. To close this gap, we now also test, if php really does render the html used in the client side testings. See tests/UI/Component/Counter/CounterClientHtmlTest.php as example.

Thx a lot.
@klees and @Amstutz

@iszmais
Copy link
Contributor Author

iszmais commented Mar 4, 2022

Hi @klees @Amstutz ,

thank you for your feedback. I Implemented all, just 2 minor things i would need your approval for:

  • aria-live="polite" is/was already set on the toast container so i didnt changed anything there. Is this what you where asking for or did i miss something?
  • I implemented the test to check the html for the JS testing but had to make a few additions to make it work like removing the js bind id (since this isnt static and not needed for the js testing itself) and wrap it into a header body structure (which do for the js testing to simulate the authentic live case of a registeredd js strucure). The toast container and toast itsself is rendered as asked. Hope this still fits your expectations.

Feel free to approve/merge this or to give a new response to my feedback.

Greetings,
@iszmais

@iszmais
Copy link
Contributor Author

iszmais commented Mar 8, 2022

This PR is an requirement for #4063

@iszmais
Copy link
Contributor Author

iszmais commented Mar 14, 2022

Hi @klees @Amstutz ,

pls let me know if there is anything i could help with or provide further information needed for the final re-review or feel free to merge this PR.

Greetings,
@iszmais

@Amstutz
Copy link
Contributor

Amstutz commented Mar 15, 2022

This got stuck somewhat behind the Dev-Conf., thx for the ind ping!

Thx a lot for the changes! Sorry that I missed the aria-live="polite" on the container, clearly my bad.

@Amstutz Amstutz merged commit a284e26 into ILIAS-eLearning:trunk Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement kitchen sink php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants