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.

Sign up for GitHub

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

Jump to bottom

Fixes #28056: Use ForemanModal in Katello modals #8384

Merged
merged 1 commit into from
Jan 22, 2020
Merged

Fixes #28056: Use ForemanModal in Katello modals #8384

merged 1 commit into from
Jan 22, 2020

Conversation

jeremylenz
Copy link
Member

@jeremylenz jeremylenz commented Oct 16, 2019 •
edited
Loading

Converting some existing Katello modals to the new ForemanModal component.

Depends on:

* [RM #27853 - Integrate modals with Redux store] [merged] (theforeman/foreman#7105)

To test this PR:

* IMPORTANT: In Foreman repo, check out branch jeremylenz/27853-modals-redux

  • Make sure your Foreman package.json lists "@theforeman/vendor": "^2.15.7"
  • In Foreman repo, run npm install

@theforeman-bot
Copy link

Issues: #28056

@jeremylenz jeremylenz force-pushed the 28056-foremanmodal branch 2 times, most recently from 2d0b8b9 to 66822ef Compare November 6, 2019 19:48
@jeremylenz
Copy link
Member Author

jeremylenz commented Nov 6, 2019 •
edited
Loading

Leaving [WIP] until Foreman #7105 is merged, but can be reviewed now by checking out that branch.
Update 1/2/20: merged; can test on latest Foreman develop

@jeremylenz
Copy link
Member Author

Katello test failures are expected until Foreman #7105 is merged

@jeremylenz jeremylenz force-pushed the 28056-foremanmodal branch 2 times, most recently from 6481e00 to 04d2d69 Compare November 13, 2019 16:17
@jeremylenz jeremylenz force-pushed the 28056-foremanmodal branch 2 times, most recently from 76ddfa1 to d0034d0 Compare November 14, 2019 19:06
@jeremylenz
Copy link
Member Author

@MariaAga Updated again. let me know if you see any more leftovers.

@jeremylenz jeremylenz force-pushed the 28056-foremanmodal branch 2 times, most recently from a5e3fa4 to fe05f7c Compare November 19, 2019 19:21
@jeremylenz
Copy link
Member Author

@MariaAga added back the missing deleteModal references

@jeremylenz jeremylenz mentioned this pull request Dec 11, 2019
Merged
@jeremylenz jeremylenz force-pushed the 28056-foremanmodal branch 2 times, most recently from c5310a2 to 5cc1091 Compare December 16, 2019 18:32
@jeremylenz
Copy link
Member Author

Updated to not dispatch addModal action since ForemanModal takes care of it now. Also rebased to latest master.

@jeremylenz jeremylenz marked this pull request as ready for review December 17, 2019 18:14
@jeremylenz
Copy link
Member Author

Katello test failures should go away once Foreman #7105 is merged

@jeremylenz jeremylenz changed the title [WIP] Fixes #28056: Use ForemanModal in Katello modals Fixes #28056: Use ForemanModal in Katello modals Jan 2, 2020
@jeremylenz
Copy link
Member Author

Rebased to latest master

@jeremylenz
Copy link
Member Author

Rebased

@jeremylenz
Fixes #28056: Use ForemanModal in Katello modals
61c557e 
Convert ManageManifestModal to ForemanModal
Manage modal ID with a constant
Convert DeleteManifestModal to ForemanModal
Change DeleteManifestModalText to JSX
update tests & snapshots
fix deleteManifestModal error
sharvit
sharvit approved these changes Jan 15, 2020
View reviewed changes
Copy link
Contributor

@sharvit sharvit left a comment

Choose a reason for hiding this comment

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

Thanks @jeremylenz, LGTM 👍

@jlsherrill jlsherrill merged commit 7ced8f2 into Katello:master Jan 22, 2020
@jeremylenz jeremylenz deleted the 28056-foremanmodal branch June 8, 2020 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlsherrill jlsherrill jlsherrill approved these changes

@sharvit sharvit sharvit approved these changes

@MariaAga MariaAga MariaAga approved these changes

Assignees
No one assigned
Labels
Needs testing Not yet reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jeremylenz @theforeman-bot @jlsherrill @sharvit @MariaAga