Fixes #39134 - Add bulk modal state helper to core#10893
Fixes #39134 - Add bulk modal state helper to core#10893ofedoren merged 1 commit intotheforeman:developfrom
Conversation
b73f573 to
e2edca5
Compare
ebccbac to
a76295c
Compare
|
Not sure if anything else needs to be added to make it available in 'foremanReact' directory for plugins. |
Lukshio
left a comment
There was a problem hiding this comment.
Hi, thank you for bringing this up.
I think that better place for this helper will be inside in file BulkModalStateHelper.js foreman/webpack/assets/javascripts/react_app/common
Also I would like to add some inline comments and description of this PR to define usage of this helper. It should be used only in situations where useState hook is not available, ex. when modal is mounted by global fill or cases like that.
a76295c to
4872f14
Compare
|
Thank you for the feedback!
Done.
I tried to add a descriptive comment but I am not sure if I grasped the technical depth correctly. @Lukshio can you please double check and correct where necessary. |
|
Thank you, looks good now, test failures are not related. |
|
It might be my OCD or misunderstanding, but: The PR refs a ticket that was release and is even in a different project, I'd advise to create a new one and link them, especially if we plan to refactor Katello to import this instead of using what's there already, since it'll require a new ticket which logically follows this one. Otherwise it'll be a bunch of refs and a bit chaotic. Also, the fix states |
Good point. I'll do that.
Well, it does replace it in Katello. You are right, I should have put more attention to the commit message. It is confusing the way it is. Thanks @ofedoren |
The fix was originally implemented in Katello but can also be used for other plugins, so it makes more sense to have it in Foreman core: Katello/katello#11626
4872f14 to
951e90f
Compare
|
I have added a new issue, linked it to the Katello issue and rephrased the commit message. |
ofedoren
left a comment
There was a problem hiding this comment.
Thanks, @nadjaheitmann and @Lukshio !
The fix was originally implemented in Katello but can also be used for other plugins, so it makes more sense to have it in Foreman core:
Katello/katello#11626