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

Add text about making notification bar dismissable #3500

Merged
merged 3 commits into from
Apr 3, 2018

Conversation

pgadige
Copy link
Contributor

@pgadige pgadige commented Apr 2, 2018

Fixes #3379
Add help text to explain how to make a notification bar dismissable.

@pgadige
Copy link
Contributor Author

pgadige commented Apr 2, 2018

I've submitted a work-in-progress[WIP] pull request. I'd like @nlhkabu @aalmazan to review the text that is added, and also provide suggestions about the grammar; sentence structure; content and the placement of text. I'll appreciate your valuable feedback. Thank you!

@@ -33,6 +33,30 @@
- dismissable: Indicates notification be be dismissed. Defaults to hidden.
Copy link
Contributor

Choose a reason for hiding this comment

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

could we fix this typo at the same time "be be" should be "can be"

@@ -33,6 +33,30 @@
- dismissable: Indicates notification be be dismissed. Defaults to hidden.
- visible: Indicates a visible, non-dismissed notification
*/
/*
A notification bar can be made dismissable by adding the following to the main
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the placement of this is fine, however, I think you can include it in the same comment block :)

/*
A notification bar can be made dismissable by adding the following to the main
notification-bar div:
- .notification-bar--dismissable to it's classes
Copy link
Contributor

Choose a reason for hiding this comment

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

notification-bar div:
- .notification-bar--dismissable to it's classes
- data-controller="notification"
- data-target="notification.notification" as attributes
Copy link
Contributor

Choose a reason for hiding this comment

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

"as an attribute" ??

- data-target="notification.notification" as attributes

The data-controller and data-target are used for handling visibility,
including, adding .notification-bar--visible to notification-bar div if it has
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for , after "including"

@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 2, 2018

Thanks @pgadige! A few comments from me, but otherwise it looks good! If @aalmazan can confirm the accuracy of the text, that would be great :)

@pgadige pgadige force-pushed the notification-help-text branch from 23320f5 to c6862a1 Compare April 3, 2018 06:27
@pgadige
Copy link
Contributor Author

pgadige commented Apr 3, 2018

Thank you @nlhkabu for your valuable feedback. I've made the required changes. Please let me know of any suggestions after you've reviewed.

@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 3, 2018

Looks good to me @pgadige ! One small thing I picked up when reading it again - Javascript should be JavaScript.

@pgadige pgadige changed the title [WIP]Add text about making notification bar dismissable Add text about making notification bar dismissable Apr 3, 2018
@aalmazan
Copy link
Contributor

aalmazan commented Apr 3, 2018

Looks good to me too @pgadige 👍

@nlhkabu nlhkabu merged commit 06a564f into pypi:master Apr 3, 2018
@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 3, 2018

Thank you @pgadige ! Thanks for your review @aalmazan !

🎉 🕺 ✨

@brainwane
Copy link
Contributor

Thanks @aalmazan and @pgadige! If either/both of you would like to work on one of these, please comment there to let us know:

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

Successfully merging this pull request may close these issues.

4 participants