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

fix(VerticalLayout): remove div element causing flex issues #101

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

ashmortar
Copy link
Contributor

@ashmortar ashmortar commented Oct 3, 2024

Unlike the HorzontalLayout the VerticalLayout was submitting div as the component prop to the antd Form component. This causes an issue with container heights, overflow and margin collapse when attempting to use the vertical layout prop in either forms or form items.

For instance, if one updates the StorybookAntDJsonForm like so:

...
 return (
 +   <Form form={form} layout="vertical">
      <AntDJsonForm<typeof jsonSchema>
        uiSchema={uiSchema}
        jsonSchema={jsonSchema}
   ....

The resulting issues can be seen in all stories featuring the VerticalLayout renderer:
Screenshot 2024-10-03 at 12 31 51 PM
Screenshot 2024-10-03 at 12 32 18 PM

however, the same issue is not present in the HorizontalLayout

Screenshot 2024-10-03 at 12 33 16 PM

After investigation this appears to have been caused by the component prop being supplied to Form in the Vertical layout being a div rather than false as it is in HorizontalLayout. This update brings parity to the two components in terms of dom structure expectations and fixes the issues with vertical forms.

Screenshot 2024-10-03 at 12 36 36 PM

@ashmortar ashmortar changed the title remove props on every render, remove div breaking control flow Remove unnecessary div Oct 3, 2024
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.80%. Comparing base (77af92c) to head (29848b6).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
- Coverage   74.85%   74.80%   -0.05%     
==========================================
  Files          38       38              
  Lines         505      504       -1     
  Branches       96       96              
==========================================
- Hits          378      377       -1     
  Misses         90       90              
  Partials       37       37              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ashmortar ashmortar force-pushed the b/PH-1572/remove-div-breaking-flex-layouts branch from 852b35b to a406e26 Compare October 3, 2024 19:25
@ashmortar ashmortar changed the title Remove unnecessary div fix(VerticalLayout): remove div element causing flex issues Oct 3, 2024
@ashmortar ashmortar marked this pull request as ready for review October 3, 2024 19:37
@ashmortar ashmortar requested a review from DrewHoo October 3, 2024 19:45
Copy link
Contributor

@DrewHoo DrewHoo left a comment

Choose a reason for hiding this comment

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

the label in vertical layout needs to be removed altogether; our types don't even allow vertical layout labels to be specified

@ashmortar ashmortar requested a review from DrewHoo October 3, 2024 21:46
@ashmortar ashmortar force-pushed the b/PH-1572/remove-div-breaking-flex-layouts branch from 831d64c to 29848b6 Compare October 3, 2024 22:07
@ashmortar ashmortar requested a review from DrewHoo October 3, 2024 22:07
Copy link
Contributor

@DrewHoo DrewHoo left a comment

Choose a reason for hiding this comment

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

LGTM!

@ashmortar ashmortar merged commit a458c7a into main Oct 3, 2024
4 of 5 checks passed
@ashmortar ashmortar deleted the b/PH-1572/remove-div-breaking-flex-layouts branch October 3, 2024 23:27
Copy link

github-actions bot commented Oct 3, 2024

🎉 This PR is included in version 1.21.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants