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

Remove obsolete notify and spinner JS code #1892

Merged
merged 15 commits into from
Oct 8, 2022
Merged

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Oct 7, 2022

Thanks to #1891 we can monitor which JS code is used or not.

This PR aims to either cover the uncovered JS files or remove them.

@mvorisek mvorisek force-pushed the improve_js_coverage branch from b34689f to 58b6c4d Compare October 7, 2022 19:09
@mvorisek mvorisek changed the title Remove obsolete JS code Remove obsolete notify and spinner JS code Oct 7, 2022
@mvorisek mvorisek force-pushed the improve_js_coverage branch from cbb6e6f to 7fe8364 Compare October 7, 2022 20:46
@mvorisek mvorisek force-pushed the improve_js_coverage branch from 1eb19b1 to f92211d Compare October 7, 2022 23:05
@mvorisek mvorisek force-pushed the improve_js_coverage branch from 055d154 to 28d4026 Compare October 8, 2022 00:14
@mvorisek mvorisek force-pushed the improve_js_coverage branch 2 times, most recently from ecb77e5 to ac76279 Compare October 8, 2022 00:59
@mvorisek mvorisek force-pushed the improve_js_coverage branch from ac76279 to 64ecc7a Compare October 8, 2022 01:14
@mvorisek mvorisek marked this pull request as ready for review October 8, 2022 01:14
@mvorisek mvorisek merged commit 85dc584 into develop Oct 8, 2022
@mvorisek mvorisek deleted the improve_js_coverage branch October 8, 2022 01:27
@mkrecek234
Copy link
Contributor

JsNotify was removed and JsToast as an identical replacement was mentioned. However, JsNotify notified the user inside the control, so for example an input field turned green, and inside the message was shown. Very compact, and with a clear link to the control that triggered the notification. I would suggest to get it back in, it served a purpose for daily use cases in my understanding. @mvorisek

@mvorisek
Copy link
Member Author

@mkrecek234 we can but it must be fully tested and the bugs must be fixed, when I was removing it, I found several bugs and not use in atk4/* ecosystem, it felt like some obsolete part to me

@mkrecek234
Copy link
Contributor

Can you re-introduce and list the bugs to be fixed? Unless other @atk4/trusted-maintainers also would prefer to remove it.

@mvorisek
Copy link
Member Author

bugs - revert this PR locally and check the demos where replaced now with Toast

what is your usercase? do you need the Toast functionality "to place notification at specific place"? do you need to call it from JS, or just from PHP?

maybe put here some example of your usage and describe what functionalitites you are missing since this change

@mkrecek234
Copy link
Contributor

bugs - can we please re-introduce and file an issue as usual?
use case - if you want a notification that is triggered by a control, with JsNotify the control itself will include the notification. This was standard behaviour for File Upload as far as I know. Looks better than having a JsToast somewhere, as the message belongs to the upload control someone just used. It is being called from PHP onUpload - I use it for onDownload and onDelete.

@mvorisek
Copy link
Member Author

all official usage was migrated, uploader docs: https://github.com/atk4/ui/pull/1892/files#diff-2b1ca070774a027b042a96b00c877f278c3b9bd7dd38fa166bd51175dc799d14R89

the bugs are comming from fact it was a non native Fomantic-UI component, I am against reintroducing the old broken JsNotify as long as there is some alternative

the position is a good point, but for that, any existing atk4 View based item can be used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants