-
Notifications
You must be signed in to change notification settings - Fork 55
Support for experimental plan management tab in UI #4549
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
Conversation
This reverts commit 074f8ff.
…ent' into hughns/plan-management
Deploying matrix-authentication-service-docs with
|
Latest commit: |
55f559e
|
Status: | ✅ Deploy successful! |
Preview URL: | https://de2da643.matrix-authentication-service-docs.pages.dev |
Branch Preview URL: | https://hughns-plan-management.matrix-authentication-service-docs.pages.dev |
5a71d49
to
26af568
Compare
…ent' into hughns/plan-management
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks alright, a few nitpicks on how things should be on the react side
useEffect(() => { | ||
calculateHeight(); | ||
|
||
const interval = setInterval(() => { | ||
calculateHeight(); | ||
}, 1000); | ||
|
||
return () => clearInterval(interval); | ||
}, [calculateHeight]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of polling, we could use a MutationObserver to recalculate. Plus, to setup the observer with a ref callback, so that we get it as soon as we have the DOM node ready.
We also don't need to track the iframe height as state to avoid re-renders, so we end up with something like
const calculateHeight = useCallback((iframe: HTMLIFrameElement) => {
const height =
iframe.contentWindow?.document.body.parentElement?.scrollHeight;
if (height) {
iframe.height = `${height}px`;
} else {
iframe.height = "500px";
}
}, []);
const ref: Ref<HTMLIFrameElement> = useCallback(
(iframe: HTMLIFrameElement) => {
calculateHeight(iframe);
if (iframe.contentWindow) {
const iframeDocument = iframe.contentWindow.document;
const observer = new MutationObserver((_mutationsList) => {
calculateHeight(iframe);
});
observer.observe(iframeDocument.body, {
childList: true,
subtree: true,
attributes: true,
});
return () => observer.disconnect();
}
},
[calculateHeight],
);
return (
<iframe
title="iframe" // no proper title as this is experimental feature
ref={ref}
onLoad={(e) => calculateHeight(e.target as HTMLIFrameElement)}
src={planManagementIframeUri}
scrolling="no"
/>
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get the observer to fire after set up. I've tried with a ResizeObserver too. Testing on Chrome and Safari. I will try again tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found two problems:
- The node corresponding to the iframe document body changed, so we weren't always observing the right thing. The fix was to re-observe after the iframe has loaded.
- Some rendering changes do not correspond to a mutation on the iframe document body DOM. This is fixed by doing an extra calculation after a delay.
I tried using the resize observer again, but it still didn't fire as expected.
|
||
/// Experimental feature to show a plan management tab and iframe | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub plan_management_iframe_uri: Option<Url>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not convinced that this has to be a URL? Like if we want to have a relative link here, it won't get accepted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose someone might want to configure it with a relative URL.
Are you thinking that it is just a string that is passed through without validation? Or are you thinking the value is parsed from config relative to http.public_base
and then the site config still return a full URL? Or something else entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm indeed thinking of just passing it as as string without validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm indeed thinking of just passing it as as string without validation
I've converted into a String
with some additional doc comments in d6dd647
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grr, I had this not submitted since yesterday
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you!
This PR adds a feature to allow a plan management tab to be added to the My Account screens.
When enabled it looks like this:
It attempts to resize the iframe to match the size of the embedded content. However, in current testing this has a habit of growing over time.