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

Widget UI 7 #3251

Merged
merged 35 commits into from
Oct 20, 2023
Merged

Widget UI 7 #3251

merged 35 commits into from
Oct 20, 2023

Conversation

fairlighteth
Copy link
Contributor

@fairlighteth fairlighteth commented Oct 18, 2023

Summary

  • modal header update
  • Resize iFrame on parent based on iFrame content height using post message
Screen.Recording.2023-10-19.at.10.28.05.mov

Iframe resize parent/widget

Screen.Recording.2023-10-19.at.12.33.14.mov

To test

@vercel
Copy link

vercel bot commented Oct 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
swap-dev ✅ Ready (Inspect) Visit Preview Oct 20, 2023 10:51am

🌃 Cosmos ↗︎

@fairlighteth
Copy link
Contributor Author

The https://jsfiddle.net/rmbf863n/11/ link contains the JS used on the parent side, to listen to postmessages. Currently it uses a wildcard domain, so something to look into in terms of security.

@fairlighteth fairlighteth marked this pull request as ready for review October 19, 2023 11:36
@fairlighteth fairlighteth requested a review from a team October 19, 2023 11:36
@@ -0,0 +1,29 @@
import { useEffect } from 'react';

export function IframeResizer() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks cool!
Have you invented it by yourself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤖 🤖 🤖

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aha!
The only thing the robot hasn't done is the code style 😉

Copy link
Collaborator

Choose a reason for hiding this comment

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

In IDEA I usually just press Cmd + option + L to fix the code style automatically

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

This is GREAT! I can't believe the widget looks so good in 340px!

It will work here as they had 345px 😂 (if only it would have more CPU and memory!)
image

// Initial height calculation and message
const sendHeightUpdate = () => {
const contentHeight = document.body.scrollHeight;
window.parent.postMessage({ type: 'iframeHeight', height: contentHeight }, '*');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we will need to create an enum for the messages. This way, we can easily see the complete list, and see exactly in the code who emits/consumes the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #3255

// Set up a MutationObserver to watch for changes in the DOM
const observer = new MutationObserver(() => {
// For simplicity, we'll send an update for any mutation, but you can add more specific checks if needed
sendHeightUpdate();
Copy link
Contributor

Choose a reason for hiding this comment

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

if there's no checks here, sendHeightUpdate fn could be used directly in the MutationObserver constructor,
Will we get false positives? Like will we post iframeHeight even if it doesn't change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's right, might run too many times. Should be fixed now in #3255

@@ -0,0 +1,29 @@
import { useEffect } from 'react';

export function IframeResizer() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find a bit strange that a component is in utils dir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Has been moved to @cowprotocol/common-utils in #3255

@fairlighteth fairlighteth changed the base branch from widget-ui-6 to develop October 20, 2023 09:03
@github-actions
Copy link
Contributor

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


2 out of 3 committers have signed the CLA.
@fairlighteth
@shoom3301
@michel
Michel seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

@fairlighteth fairlighteth merged commit 6a36d1a into develop Oct 20, 2023
3 of 6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2023
@alfetopito alfetopito deleted the widget-ui-7 branch October 23, 2023 09:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants