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

🪟🐛 Make modal scrollable #21973

Merged
merged 10 commits into from
Feb 6, 2023
Merged

🪟🐛 Make modal scrollable #21973

merged 10 commits into from
Feb 6, 2023

Conversation

flash1293
Copy link
Contributor

What

Fixes #21793

Currently, the modal component will overflow the viewport and parts of the content will be cut off. This PR allows for the modal to be limited to the currently available height and the body of the modal becoming scrollable:

Kapture.2023-01-27.at.14.32.22.mp4

How

  • Make all levels of the modal component hierarchy flex
  • Set a max-width on the wrapper centering the modal
  • Inputs form had to be adjusted because it wrapped the form element around the body and the footer so the body didn't have a chance to shrink anymore on too much content - moved it outside of the wrapper.

I checked a few other modals around the app and didn't notice any breakages, but worth checking them again in case I missed something

@flash1293 flash1293 added area/frontend Related to the Airbyte webapp ui/connector-builder labels Jan 27, 2023
@flash1293 flash1293 marked this pull request as ready for review January 27, 2023 20:52
@lmossman
Copy link
Contributor

@flash1293 this PR doesn't seem to solve the issue for me. It just locks the top of the modal to the top of my screen, but the bottom of the window still covers up the modal instead of the modal shrinking.

Here is a screen recording of what I'm seeing on this branch:

Screen.Recording.2023-01-31.at.2.01.20.PM.mov

I tried fiddling with the CSS a bit more but wasn't able to affect this behavior quickly. Are you able to reproduce what I'm seeing?

@flash1293
Copy link
Contributor Author

I think I messed something up before submitting, sorry - could you check again? Two other things I noticed:

  • Had to move the form back into the modal because otherwise it's not wrapping the submit buttons in the dom and submitting doesn't work
  • This fix doesn't work for the test input form because it nests the footer in the body due to the dom structure - I think it's fixable but it's a bit more refactoring, I will do that separately

Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

@flash1293 those changes fixed the InputForm modal behavior for me.

However, when trying out some other modals in the app, I noticed that this fix doesn't seem to work in general.

For example, the RequestConnectorModal didn't properly scroll because it has overflow: unset in its styles (this is more of a special case. Removing that line seemed to fix this)

And the CreateConnectorModal (the modal that pops up when adding a custom connector) also did not have the proper scrolling behavior. I could only fix this by adding display: contents to its Form style, similar to how you fixed the InputsForm here.

It would be nice if we had a more general fix for all of our modals that didn't require us to add styles to the forms specifically, but since our various modals are structured slightly differently I can see why that would be somewhat hard to achieve.

I'll leave it up to you to decide if you want to add the fixes I pointed out above to this PR, or to merge it as is and deal with those separately. This PR at least improves the Connector Builder modals and doesn't really make the others any worse

@flash1293
Copy link
Contributor Author

Good callout @lmossman - my thinking was that the scrolling works as long as header, body and footer and in the same parent element, but that's often not the case because of the way we do forms - I worked around this by introducing an optional wrapIn prop to the modal component which allows the caller to wrap header/body/footer into the the required form component on the same level, so scrolling should work for these two cases now as well (without css tricks)

Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

I think these changes LGTM now.

When locally testing I found one more issue in which the confirmation modal was being cut off if the viewport was too small even with the other changes. This is because the confirmation modal is wrapping the content/footer into a single container which did not have overflow.

I fixed this by setting overflow: auto on the confirmation modal content, which I pushed up to this branch here: a212870

@flash1293 feel free to adjust that fix if you want. It's a little unfortunate that the fixes in this PR don't necessarily guarantee that the modal will always be scrollable, but I think that is just a result of the fact that the Modal component children are set in so many different ways. This PR at least covers the common cases now.

@flash1293 flash1293 enabled auto-merge (squash) February 5, 2023 16:08
@flash1293 flash1293 enabled auto-merge (squash) February 5, 2023 23:05
@flash1293 flash1293 enabled auto-merge (squash) February 6, 2023 16:16
@flash1293 flash1293 merged commit e39b90f into master Feb 6, 2023
@flash1293 flash1293 deleted the flash1293/modal-scrollable branch February 6, 2023 16:40
letiescanciano added a commit that referenced this pull request Feb 6, 2023
* master:
  Discover worker starts to use API to write schema result (#21875)
  🪟 🎉  Connector Builder Landing Page (#22122)
  Fix pnpm cache path (#22418)
  Add additional shorter setup guides (#22318)
  Source Amazon Ads: fix reports stream records primary keys (#21677)
  Connector acceptance test: Fix discovered catalog caching for different configs (#22301)
  🪟🐛 Make modal scrollable (#21973)
  only compute diff if the schema discovery actually succeeded (#22377)
  Source Klaviyo: fix schema (#22071)
  🪟 🔧 Switch to `pnpm` for package managing (#22053)
  Source Sentry: turn on default availability strategy (#22303)
  Source freshdesk: deduplicate table names (#22164)
letiescanciano added a commit that referenced this pull request Feb 6, 2023
* master: (86 commits)
  Discover worker starts to use API to write schema result (#21875)
  🪟 🎉  Connector Builder Landing Page (#22122)
  Fix pnpm cache path (#22418)
  Add additional shorter setup guides (#22318)
  Source Amazon Ads: fix reports stream records primary keys (#21677)
  Connector acceptance test: Fix discovered catalog caching for different configs (#22301)
  🪟🐛 Make modal scrollable (#21973)
  only compute diff if the schema discovery actually succeeded (#22377)
  Source Klaviyo: fix schema (#22071)
  🪟 🔧 Switch to `pnpm` for package managing (#22053)
  Source Sentry: turn on default availability strategy (#22303)
  Source freshdesk: deduplicate table names (#22164)
  Update connector-acceptance-tests-reference.md (#22370)
  Update the default security groups for the EC2 runner (#22347)
  Trace refresh schema operations (#22326)
  Remove manual docker upgrades from workflows (#22344)
  Update CODEOWNERS for connector acceptance tests to connector ops (#22341)
  🐛 source: airtable - handle singleSelect types (#22311)
  Source tiktok: chunk advertiser IDs (#22309)
  🪟 🧪 E2E Tests for auto-detect schema changes (#20682)
  ...
letiescanciano added a commit that referenced this pull request Feb 6, 2023
* master: (24 commits)
  Discover worker starts to use API to write schema result (#21875)
  🪟 🎉  Connector Builder Landing Page (#22122)
  Fix pnpm cache path (#22418)
  Add additional shorter setup guides (#22318)
  Source Amazon Ads: fix reports stream records primary keys (#21677)
  Connector acceptance test: Fix discovered catalog caching for different configs (#22301)
  🪟🐛 Make modal scrollable (#21973)
  only compute diff if the schema discovery actually succeeded (#22377)
  Source Klaviyo: fix schema (#22071)
  🪟 🔧 Switch to `pnpm` for package managing (#22053)
  Source Sentry: turn on default availability strategy (#22303)
  Source freshdesk: deduplicate table names (#22164)
  Update connector-acceptance-tests-reference.md (#22370)
  Update the default security groups for the EC2 runner (#22347)
  Trace refresh schema operations (#22326)
  Remove manual docker upgrades from workflows (#22344)
  Update CODEOWNERS for connector acceptance tests to connector ops (#22341)
  🐛 source: airtable - handle singleSelect types (#22311)
  Source tiktok: chunk advertiser IDs (#22309)
  🪟 🧪 E2E Tests for auto-detect schema changes (#20682)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connector builder: Inputs modal can grow higher than the screen
2 participants