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

Please add multi modal widget #132

Closed
lucatrv opened this issue Jun 6, 2023 · 6 comments
Closed

Please add multi modal widget #132

lucatrv opened this issue Jun 6, 2023 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@lucatrv
Copy link
Contributor

lucatrv commented Jun 6, 2023

The current modal widget is restricted to only two possible states: visible and not-visible. This makes quite tricky to implement modals with multiple possible overlays.
It would be very useful to have a multi_modal widget, or otherwise to extend the current modal widget. This widget could accept for instance a show_modal: Option<usize> parameter, and then an array of overlay contents. In case of show_modal = None no overlay would be shown, and in case of show_modal = Some(i) the content[i] overlay would be shown.

@lucatrv lucatrv changed the title Please add multi modal feature Please add multi modal widget Jun 6, 2023
@genusistimelord
Copy link
Collaborator

this sounds like a decent change!

@genusistimelord genusistimelord added enhancement New feature or request help wanted Extra attention is needed labels Jun 6, 2023
@lucatrv
Copy link
Contributor Author

lucatrv commented Aug 14, 2023

@genusistimelord please review PR #169. I think it is a better solution to support multiple overlays: it removes the show_modal boolean altogether and changes overlay into an Option. This way it allows maximum flexibility in choosing whether to display an overlay or not (by passing None). The PR is still partial (I updated only the functional code, not the comments / documentation), but would like to get your feedback on this possible approach before completing it.

Actually I think there is even a simpler solution: by removing the Option from overlay in PR #169, the user could simply show directly underlay (without calling modal) when the overlay should not be shown. This would actually be my preference, but I ask your opinion before implementing it. With this implementation, modal would always have the overlay visible when called.

If you do not like the two above implementations, then a new multimodal widget should be added accepting a Vec of overlays and an usize. If the usize corresponds to an element of the Vec of overlays, that overlay should be shown, otherwise no overlay should be shown. IMHO this is not needed though, as the above two implementations (in particular the second one) are simpler and allow maximum flexibility.

@genusistimelord
Copy link
Collaborator

yeah just fix the clippy issue as it should be good to merge.

@genusistimelord
Copy link
Collaborator

Oh and add an example showing multiple modals in usage would be nice too.

@lucatrv
Copy link
Contributor Author

lucatrv commented Aug 16, 2023

Hi @genusistimelord I saw that you already merged PR #169 and fixed the clippy issue yourself. Sorry I did not answer before but yes I am adding a multiple modals example, just hold on and I am going to PR it. Before that I am also going to fix the modal's comments and documentation.

@genusistimelord
Copy link
Collaborator

ok thats fine. PR whenever your ready. I will close this since the last one fixed the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants