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

🐛 Decouple form actions from ApplicationForm #1776

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

rszwajko
Copy link
Collaborator

Split ApplicationForm into a data hook and view component. Use extracted submit and cancel actions inside Modal's footer.

Resolves: #1708

@rszwajko rszwajko requested review from ibolton336 and sjd78 March 14, 2024 17:14
@rszwajko rszwajko marked this pull request as ready for review March 18, 2024 17:41
@rszwajko
Copy link
Collaborator Author

After

image

@ibolton336 ibolton336 closed this Mar 19, 2024
@ibolton336 ibolton336 reopened this Mar 19, 2024
@rszwajko rszwajko force-pushed the scrollInNewApp branch 2 times, most recently from 6d892b9 to 25545b1 Compare March 19, 2024 18:57
@rszwajko rszwajko requested a review from ibolton336 March 19, 2024 19:03
Copy link
Member

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

Looks ok as-is, but just a thought about how you could manage the buttons from the form itself. Not a blocker.

@ibolton336 ibolton336 added the cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch. label Mar 20, 2024
@ibolton336 ibolton336 added this to the v0.3.1 milestone Mar 21, 2024
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

After looking at what the createPortal-to-a-ref-in-the-Modal-footer would look like, and lots of discussion about it, the current setup is ok with me.

Going forward, we should think about if other tweaks to this approach would be good to apply across all form modals. I'm sure this will not be the last "keep the buttons sticky to the bottom" kind of request we will encounter.

@rszwajko
Copy link
Collaborator Author

For reference: version that uses portal to display buttons in the parent modal is available here -> rszwajko@ec93ae7

Split ApplicationForm into a data hook and view component.
Use extracted submit and cancel actions inside Modal's footer.

Resolves: konveyor#1708
Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Signed-off-by: Radoslaw Szwajkowski <[email protected]>
@ibolton336 ibolton336 merged commit bb7fc47 into konveyor:main Mar 22, 2024
7 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 22, 2024
Split ApplicationForm into a data hook and view component. Use extracted
submit and cancel actions inside Modal's footer.

Resolves: #1708

---------

Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Co-authored-by: Ian Bolton <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
ibolton336 added a commit that referenced this pull request Mar 22, 2024
Split ApplicationForm into a data hook and view component. Use extracted
submit and cancel actions inside Modal's footer.

Resolves: #1708

---------

Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Co-authored-by: Ian Bolton <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>

Signed-off-by: Radoslaw Szwajkowski <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
Co-authored-by: Radoslaw Szwajkowski <[email protected]>
Co-authored-by: Ian Bolton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch.
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[BUG] Scrollable Modal - Visible Primary and Secondary Button
3 participants