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

Added array to snackbar qml #370

Merged
merged 9 commits into from
Mar 25, 2022
Merged

Added array to snackbar qml #370

merged 9 commits into from
Mar 25, 2022

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Mar 8, 2022

🎉 New feature

Summary

if we start getting too many notifications at once to show in the snackbar, this logic will display them consecutively.

ESC will close the popup, otherwise with the mouse click some of the popup will close inmediately if you are clicking several times to configure something.

Build on top of #369

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@ahcorde ahcorde self-assigned this Mar 8, 2022
@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #370 (f7daa20) into ign-gui3 (fc2f870) will decrease coverage by 0.13%.
The diff coverage is 38.46%.

❗ Current head f7daa20 differs from pull request most recent head 1b58a8c. Consider uploading reports for the commit 1b58a8c to get more accurate results

@@             Coverage Diff              @@
##           ign-gui3     #370      +/-   ##
============================================
- Coverage     65.96%   65.83%   -0.14%     
============================================
  Files            29       29              
  Lines          3191     3202      +11     
============================================
+ Hits           2105     2108       +3     
- Misses         1086     1094       +8     
Impacted Files Coverage Δ
include/ignition/gui/MainWindow.hh 100.00% <ø> (ø)
src/plugins/screenshot/Screenshot.cc 30.12% <0.00%> (-3.22%) ⬇️
src/plugins/teleop/Teleop.cc 83.19% <37.50%> (-1.77%) ⬇️
src/plugins/image_display/ImageDisplay.cc 24.07% <66.66%> (+0.80%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c14f379...1b58a8c. Read the comment docs.

@chapulina chapulina added the OOBE 📦✨ Out-of-box experience label Mar 8, 2022
Base automatically changed from ahcorde/3/snackbar to ign-gui3 March 9, 2022 20:01
@chapulina chapulina added the 🏰 citadel Ignition Citadel label Mar 10, 2022
@ahcorde ahcorde requested a review from chapulina March 10, 2022 20:09
@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 10, 2022

@chapulina

  • Added a Dismiss button
  • Aligned text
  • used bold to highlight some relevant information such as: topics or paths

@chapulina chapulina linked an issue Mar 11, 2022 that may be closed by this pull request
@chapulina
Copy link
Contributor

Added a Dismiss button

Thanks! I was thinking of making that optional though. Most implementations have buttons as optional, see

Usually the button text is configurable and there's a way to register a callback when the button is clicked. I don't think we need all these features just yet, but it would be good to:

  • structure the API in a way that it can be extended later
  • the button should be optional for now, so messages that aren't very important have a timeout and can be dismissed by the usual means (like subscription to topics), but messages that are important have a button that must be clicked to dismiss (like a failure to save a file).
  • make the button flat like in the implementations above (from the spec "Don’t use a filled or elevated button in a snackbar, as it draws too much attention.")

Aligned text

I think it would be better to follow the spec and align the text to the left like it was already.

@chapulina
Copy link
Contributor

@ahcorde , I have some suggestions on #375, let me know what you think

@ahcorde ahcorde mentioned this pull request Mar 25, 2022
@chapulina
Copy link
Contributor

Argh, Bionic is broken with

   qrc:/qml/IgnSnackBar.qml:48 Cannot assign to non-existent property "horizontalPadding"

That's something I added on #375. Let me find an alternative

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM once CI comes back ok

@chapulina
Copy link
Contributor

@osrf-jenkins run tests please

@chapulina
Copy link
Contributor

Bionic is happy now, the webhooks have been acting up all day. Merging.

@chapulina chapulina merged commit 3cd66a8 into ign-gui3 Mar 25, 2022
@chapulina chapulina deleted the ahcorde/3/snackbarArray branch March 25, 2022 21:31
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel OOBE 📦✨ Out-of-box experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification snackbar
3 participants