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: add auto resizing for plugins [LIBS-487] #1355

Merged
merged 6 commits into from
Sep 28, 2023
Merged

Conversation

tomzemp
Copy link
Member

@tomzemp tomzemp commented Aug 22, 2023

Implements LIBS-487

Note this change requires updates in both app-runtime and in app-platform (see also app-platform PR: dhis2/app-platform#814)


Key features

  1. Adds option to autoresize plugin height/width based on its contents. If a fixed height/width is provided that will be used. Otherwise, the plugin will by default resize to its contents.

Description

See ticket


Checklist

  • Have written Documentation
    • a note has been added to the documentation for Plugin component
  • Has tests coverage
    • Tests will be written when we decide we want to move forward with Plugin wrappers

Known issues

As noted on the ticket, I'm not totally sure what the behaviour should be for autoresizing of width.


Screenshots

Top/Orange box has a plugin with autoresizing (of both width/height); Bottom/Blue box has fixed width

autoResizePlugins

@tomzemp tomzemp changed the title Libs 487/auto resizing feat: add auto resizing for plugins [LIBS-487] Aug 22, 2023
@tomzemp tomzemp marked this pull request as ready for review August 22, 2023 11:21
@tomzemp tomzemp mentioned this pull request Aug 22, 2023
}: {
pluginSource?: string
pluginShortName?: string
propsToPass: any
}): JSX.Element => {
const iframeRef = useRef<HTMLIFrameElement>(null)

// we do not know what is being sent in passed props, so for stable reference, memoize using JSON representation
const propsToPassNonMemoizedJSON = JSON.stringify(propsToPassNonMemoized)
Copy link
Member Author

Choose a reason for hiding this comment

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

stable reference for props is handled in a more thought-out-manner in #1351

Copy link
Contributor

@kabaros kabaros left a comment

Choose a reason for hiding this comment

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

I am thinking though, maybe it makes more sense to have a min-height property? and kind of related, do we need the div to have overflow-y: scroll or auto or will that work out of the box

@tomzemp
Copy link
Member Author

tomzemp commented Aug 28, 2023

I am thinking though, maybe it makes more sense to have a min-height property?

As in, have min-height if you want to use auto-resizing for height, but also have some minimum height? I was imagining that there'd be use cases where people would want a fixed height (e.g. on the dashboard), but I think that min-height/max-height combined with auto-resizing would also make sense.

and kind of related, do we need the div to have overflow-y: scroll or auto or will that work out of the box

this worked out of the box for me, but I only tested in chrome and firefox, so could look into that more.

@kabaros
Copy link
Contributor

kabaros commented Aug 28, 2023

I am thinking though, maybe it makes more sense to have a min-height property?

As in, have min-height if you want to use auto-resizing for height, but also have some minimum height? I was imagining that there'd be use cases where people would want a fixed height (e.g. on the dashboard), but I think that min-height/max-height combined with auto-resizing would also make sense.

yeah this is different requirement probably .. so can leave it for now until there is a need.

@tomzemp tomzemp merged commit 4136bd5 into alpha Sep 28, 2023
7 checks passed
@tomzemp tomzemp deleted the LIBS-487/auto-resizing branch September 28, 2023 07:59
@dhis2-bot
Copy link
Contributor

@dhis2-bot
Copy link
Contributor

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

Successfully merging this pull request may close these issues.

3 participants