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: Interface for Toast Container #4049

Merged
merged 1 commit into from
Feb 24, 2022
Merged

UI: Interface for Toast Container #4049

merged 1 commit into from
Feb 24, 2022

Conversation

iszmais
Copy link
Contributor

@iszmais iszmais commented Feb 16, 2022

This PR proposes to introduce the UI-Component for implementing Containers for Toasts.
Toast Container:
The Toast Container is a wrapper without direct visual represantation which centralizes the handling and accessebility of Toast components.

A Toast Container can appear in the top right corner , overlaying the ILIAS UI. It can contain and handle Toasts Components and present them to a screen reader. There is not direct way to interact with the Container itself. If the Container does not contain any Toast Components it has no effect on the visual page.


The new concept of the Container has the following effect on the existing concept of the Toast (See Toasts):

A Toast can appear in the top right corner, overlaying the ILIAS UI

This ability is omitted since the container decides about the position of apperance.

@iszmais
Copy link
Contributor Author

iszmais commented Feb 16, 2022

@klees @Amstutz

As planned in our meeting i implemented a toast container interface.
I would appreciate if you could take a short look if there is some major issues or differences to your expectation which coudl possibly be prevented before the Jour Fixe

Greetings,
@iszmais

@iszmais iszmais requested review from alex40724, Amstutz and klees and removed request for alex40724 February 16, 2022 13:54
@klees
Copy link
Member

klees commented Feb 16, 2022

It's gonna be me, @Amstutz isn't available this week...

@iszmais iszmais removed the request for review from Amstutz February 16, 2022 13:59
@PurHur
Copy link
Contributor

PurHur commented Feb 16, 2022

In the material design of Google which defined the wording "Toast" a toast is a notification without an action.
In the code i see the possibility to attach a link to the toast. Wouldnt that make the Toast to a Snackbar instead? 🍞🍫

Reference: https://material.io/archive/guidelines/components/snackbars-toasts.html

@mjansenDatabay mjansenDatabay added the php Pull requests that update Php code label Feb 16, 2022
@mjansenDatabay
Copy link
Contributor

In the material design of Google which defined the wording "Toast" a toast is a notification without an action. In the code i see the possibility to attach a link to the toast. Wouldnt that make the Toast to a Snackbar instead? breadchocolate_bar

Reference: https://material.io/archive/guidelines/components/snackbars-toasts.html

Honestly a valid point @PurHur, but the term was defined by the Page Layout Revision (and accepted by the JourFixe) as @iszmais presented this some months ago. Of course this could be renamed in the future if we all decide the name is not appropriate. This has to be discussed with the community, new PRs have to be provided and presented in the JourFixe. This might happen, but not for ILIAS 8 (except if someone wants to provide more 💵 and the ILIAS 8 release will be delayed again).

@PurHur
Copy link
Contributor

PurHur commented Feb 16, 2022

I understand your comment. I was just writing that comment, because it was the fist time i encountered these code lines. I understand that this is not the original PR, but i cant look over all comments on all different platforms for the illias development, this is outside of my work scope. This would take tooooo much time. So i just wrote it here to give at least some feedback which could be read. I dont expect that this have any inpact, if you dont like it.
Take that comment as feedback for your JourFix and discuss it there. (Sadly i will not have time monday again)

Copy link
Member

@klees klees left a comment

Choose a reason for hiding this comment

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

LGTM in general, thanks a lot. I left some inline comments, hope it is ok that I did not leave a fully-fledged review...

src/UI/Component/Toast/Container.php Outdated Show resolved Hide resolved
src/UI/Component/Toast/Factory.php Outdated Show resolved Hide resolved
src/UI/Component/Toast/Factory.php Outdated Show resolved Hide resolved
src/UI/Component/Toast/Factory.php Outdated Show resolved Hide resolved
src/UI/Component/Toast/Factory.php Outdated Show resolved Hide resolved
@iszmais
Copy link
Contributor Author

iszmais commented Feb 17, 2022

Thx @klees for the review,
i personally actual prefer it that way 😄

Implemented all changed and quashed again.

Grretings,
@iszmais

@matthiaskunkel
Copy link
Member

Jour Fixe, 21 FEB 2022 : We highly appreciate this suggestion and accept the PR for ILIAS 8.

@iszmais
Copy link
Contributor Author

iszmais commented Feb 22, 2022

@matthiaskunkel thanks for the appreciation!

@klees feel free to merge or to give some minor feedback if needed.

@mjansenDatabay mjansenDatabay changed the title UI Components: Interface for Toast Container UI: Interface for Toast Container Feb 23, 2022
@Amstutz
Copy link
Contributor

Amstutz commented Feb 24, 2022

Hi @iszmais

Thx a lot for providing the interface along with the examples.

This seems all fine. We merge this to proceed to the next step. We are looking forward to review the implementation.

Thx
@klees and @Amstutz

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

Successfully merging this pull request may close these issues.

6 participants