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

Updated alerts to use toasts #419

Merged
merged 8 commits into from
Nov 25, 2020
Merged

Conversation

ajstewart
Copy link
Contributor

I've updated the alerts to use toasts instead, some screenshots:

Screen Shot 2020-11-19 at 14 13 56

Screen Shot 2020-11-19 at 14 14 28

Screen Shot 2020-11-19 at 14 14 45

Though I have one problem, the toast doesn't take z-index precedence over Aladin-lite (see screenshot) @marxide would you know how to solve this?:

Screen Shot 2020-11-19 at 14 13 59

Also I noticed the notifications don't appear on the source_detail.html template unless the toasts script is explicitly loaded there. I imagine that this is why the alerts one was there in the first place, but I'm guessing this is also something to do with with Aladin or JS9 on this page?

Fixes #260.

@ajstewart ajstewart added enhancement New feature or request UI User Interface labels Nov 19, 2020
@ajstewart ajstewart self-assigned this Nov 19, 2020
@github-actions github-actions bot added this to In progress in Pipeline Backlog Nov 19, 2020
srggrs
srggrs previously approved these changes Nov 20, 2020
Copy link
Contributor

@srggrs srggrs left a comment

Choose a reason for hiding this comment

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

Great!

Pipeline Backlog automation moved this from In progress to Reviewer approved Nov 20, 2020
@ajstewart
Copy link
Contributor Author

I'll wait to see if @marxide has any solution for the Aladin z-index.

Pipeline Backlog automation moved this from Reviewer approved to Review in progress Nov 20, 2020
@ajstewart
Copy link
Contributor Author

I managed to track down the z-index bug, it was because the toast is shown inside the alert-wrapper div which means the toast could only inherit a z-index no larger than it's parent container. This was fixed by adding the high z-index to the alert-wrapper now renamed to toast-wrapper.

Screen Shot 2020-11-24 at 5 24 06 pm

So this is now ready to go.

Copy link
Contributor

@srggrs srggrs left a comment

Choose a reason for hiding this comment

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

Just fix committed conflicts then all good!

CHANGELOG.md Outdated
Comment on lines 124 to 128
<<<<<<< HEAD
- [#408](https://github.com/askap-vast/vast-pipeline/pull/408) feat: use forced_phot dependency instead of copied code.
- [#407](https://github.com/askap-vast/vast-pipeline/pull/407) fix, model: modified 2-epoch metric calculation.
=======
>>>>>>> 381c2a269ea04dba17bab187f4cf35a240949e7a
Copy link
Contributor

Choose a reason for hiding this comment

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

fix conflict!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How did I miss that! I guess that's why I wait for reviews 😆 Fixed in e24ba16.

@ajstewart
Copy link
Contributor Author

Merging with readme fix.

@ajstewart ajstewart merged commit 51aaae7 into master Nov 25, 2020
Pipeline Backlog automation moved this from Review in progress to Done Nov 25, 2020
@ajstewart ajstewart deleted the issue-260-use-toast-notifications branch November 25, 2020 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request UI User Interface
Projects
Development

Successfully merging this pull request may close these issues.

Rewrite UI alerts using Bootstrap Toast
2 participants