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

Unify success alerts #1469

Merged
merged 28 commits into from
Feb 3, 2022
Merged

Conversation

ShaiahWren
Copy link
Contributor

@ShaiahWren ShaiahWren commented Jan 7, 2022

See issue: https://issues.redhat.com/browse/AAH-1236

Assignment: Update success alerts to fit modular format outlined in the documentation. Also see additional documentation for general messages.

This pr updates all alerts messages across the application according to the above documentation. In some cases, missing alerts were added.

@ShaiahWren ShaiahWren force-pushed the unify-success-alerts branch 3 times, most recently from 499bdd0 to f12761b Compare January 10, 2022 22:56
brumik
brumik previously requested changes Jan 11, 2022
Copy link
Contributor

@brumik brumik left a comment

Choose a reason for hiding this comment

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

In global looks good, but I have concerns regarding to the translation. See the comments bellow.

src/containers/execution-environment-detail/base.tsx Outdated Show resolved Hide resolved
src/containers/settings/user-profile.tsx Outdated Show resolved Hide resolved
src/containers/task-management/task-list-view.tsx Outdated Show resolved Hide resolved
CHANGES/1236.task Outdated Show resolved Hide resolved
@himdel
Copy link
Collaborator

himdel commented Jan 12, 2022

@ShaiahWren you may have missed some ... Successfully deleted user. in delete-user-modal is unchanged, probably belongs here as well?

(Are you splitting the effort in multiple PRs or should I try checking for more missing success messages?)

@ShaiahWren
Copy link
Contributor Author

@himdel AlertType now accepts JSXElement or string for the title, however, as you can see the bold doesn't seem to be showing up in the alerts. Is it my syntax?

I will continue to work on the other items. Thank you for the thorough review. :)

@ShaiahWren ShaiahWren merged commit 7d7c037 into ansible:master Feb 3, 2022
@ShaiahWren ShaiahWren deleted the unify-success-alerts branch February 3, 2022 14:59
@himdel himdel mentioned this pull request Feb 4, 2022
2 tasks
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 5, 2022
noticed in ansible#1738, looks like this was introduced in ansible#1469 to replace "Successfully deleted collection."

we're now using the same success message in `deleteCollection` and `deleteCollectionVersion`, which is misleading (they didn't just delete one version), and broken since there is no version info, just the `v`
updating to remove the version info
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 5, 2022
noticed in ansible#1738, looks like this was introduced in ansible#1469 to replace "Successfully deleted collection."

we're now using the same success message in `deleteCollection` and `deleteCollectionVersion`, which is misleading (they didn't just delete one version), and broken since there is no version info, just the `v`
updating to remove the version info

No-Issue
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 5, 2022
noticed in ansible#1738, looks like this was introduced in ansible#1469 to replace "Successfully deleted collection."

we're now using the same success message in `deleteCollection` and `deleteCollectionVersion`, which is misleading (they didn't just delete one version), and broken since there is no version info, just the `v`
updating to remove the version info

No-Issue
himdel added a commit that referenced this pull request Apr 7, 2022
* deleteCollection - don't mention version when deleting the whole

noticed in #1738, looks like this was introduced in #1469 to replace "Successfully deleted collection."

we're now using the same success message in `deleteCollection` and `deleteCollectionVersion`, which is misleading (they didn't just delete one version), and broken since there is no version info, just the `v`
updating to remove the version info

No-Issue

* collection.js test - fix expected alert message to match
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.

None yet

3 participants