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

Add support to defender deploy #416

Merged
merged 29 commits into from
Dec 16, 2024
Merged

Conversation

MCarlomagno
Copy link
Member

@MCarlomagno MCarlomagno commented Dec 2, 2024

Description

Adds a modal that connects the wizard with the Defender Deploy Plugin (currently only supported in remix).

The modal contains an Iframe that points to the live version of the remix plugin.

@MCarlomagno MCarlomagno changed the title [WIP] Add support to defender deploy Add support to defender deploy Dec 11, 2024
@MCarlomagno MCarlomagno requested a review from ericglau December 11, 2024 16:19
Copy link
Member

@ericglau ericglau left a comment

Choose a reason for hiding this comment

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

Looking good so far, thanks! Just left some initial comments.

I also notice if I close the iframe and re-open it, the API key needs to be entered again. Do we have a way to persist that information just for the current session? Or avoid truly closing it, but just hide it.

packages/ui/src/standalone.css Outdated Show resolved Hide resolved
packages/ui/src/App.svelte Outdated Show resolved Hide resolved

const dispatch = createEventDispatcher();

// listens to contract changes and posts them to the defender deploy iframe.
// we debounce the call to avoid sending too many messages while the user is editing.
const debouncedPostMessage = debouncer((contract) => {
Copy link
Member

Choose a reason for hiding this comment

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

While this is waiting for the debounce timeout, there is no indication to the user that the iframe is waiting to be updated. The iframe just gets updated 600 ms later, which "feels" like lag.

@makiopen Any suggestions on how to add some indication that the iframe is pending an update?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can display some loading state with some "compiling contract" message. We would need to make another post message (instantly) to the iframe to activate the loading state until the debounced message arrives

Copy link
Collaborator

Choose a reason for hiding this comment

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

even a spinner would be enough tbh

@MCarlomagno
Copy link
Member Author

MCarlomagno commented Dec 12, 2024

I also notice if I close the iframe and re-open it, the API key needs to be entered again. Do we have a way to persist that information just for the current session? Or avoid truly closing it, but just hide it.

@ericglau Thanks! I ended up hiding the iframe instead of conditionally loading the component, a bit hacky but works

@ericglau
Copy link
Member

The AI Assistant icon also seems missing, not sure where it went. Perhaps due to the formatting changes.

@makiopen
Copy link
Collaborator

some z-index issues, brought it back

Copy link
Member

@ericglau ericglau left a comment

Choose a reason for hiding this comment

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

Added title (hover) text for "Copy to Clipboard" and "Open in Remix" since they are now icon-only and it may not be obvious what they do.

LGTM, thanks!

@MCarlomagno MCarlomagno merged commit b829f16 into master Dec 16, 2024
9 checks passed
@MCarlomagno MCarlomagno deleted the add-support-to-defender-deploy branch December 16, 2024 14:02
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.

3 participants