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

feat: plugin sizing updates #902

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

KaiVandivier
Copy link
Contributor

@KaiVandivier KaiVandivier commented Dec 17, 2024

Address LIBS-723

Requires app-runtime changes (dhis2/app-runtime#1398)

  • By default, stretches the plugin width to behave (mostly) like a regular div with default width: auto
    • iframes can't actually do width: auto, so width: 100% is currently used -- margins don't work as expected in this case though
    • width: auto behavior could work with a <div> around the iframe, where the div uses the auto width and the iframe uses width: 100% to match that size
  • Adds and uses a className prop to <Plugin> and the underlying iframe
  • If a contentWidth prop is used on the Plugin component, then the width of the iframe will be based off the contents inside
  • The animation loop to check the content width and send that to the parent is removed
  • Removes 20px bottom padding in iframe, since offsetHeight should measure a scrollbar height

I used patch-package to make it easier to test this out with app-runtime changes. I made 6 test cases in a component in the simple app to use:

  1. Default "width: auto" behavior
  2. Using height: 100%
  3. Using fixed heights and widths
  4. Using content-driven sizing
  5. Flexing horizontally
  6. Flexing vertically
Screen.Recording.2024-12-17.at.14.20.12.mov

To do and known issues
Before review:

  • Resizing stops working if the plugin hits its internal error boundary (observers and callbacks probably need to be set up again). Update: actually, I'm unable to reproduce this -- seems to be working 🤷
  • Try out div around iframe to get default 'auto' width of divs (and iframe has width(/height?) 100% by default). Update: tried this out, and it makes the right behavior for width: auto, but breaks other height behaviors. I don't think it's worth doing
  • Add code comments to explain sizing control flow what's going on
  • Write docs and guidance ⭐

After review:

  • Remove test components in the simple example app
  • Remove console logs
  • Remove patch package

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
5 New issues
5 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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

Successfully merging this pull request may close these issues.

1 participant