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

Interactives UX: Finalize insert code #5499

Merged
merged 6 commits into from
Jul 20, 2019

Conversation

jmif
Copy link
Contributor

@jmif jmif commented Jul 17, 2019

Finalize insert code UX

@jmif jmif force-pushed the interactives-insert-code-finalize-ux branch from e17b888 to ce115d3 Compare July 18, 2019 00:37
@jmif jmif force-pushed the interactives-insert-code-finalize-ux branch from 0de8146 to 149fd58 Compare July 18, 2019 01:02
return
}

window.requestAnimationFrame(() => this.codemirror.setSize('100%', '100%'))
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a way to defer the setSize method call. I'm interested in what the pros are of this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, this queues the changes until right before the browsers next render. It's a performance optimization (often used in animations) so allow visual updates to only run on browser repaints. This is usually synced with the browser frame rate.

Copy link
Contributor

@AndrewJakubowicz AndrewJakubowicz left a comment

Choose a reason for hiding this comment

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

This looks great in general. Left a question. Some of the sass constants could be stored as variables in the ozaria varables.sass.

I also tried to test this using proxy mode with the following interactive: http://localhost:3000/interactive/5d0beffa9339c40022a56557?code-language=python

I get the error:

TypeError: "this.selectedAnswer is undefined"
    artUrl index.vue:155

and the page doesn't load. Is this expected and handled in another PR?

@jmif
Copy link
Contributor Author

jmif commented Jul 19, 2019

@AndrewJakubowicz good catch, this was not handled in another PR. Fixed.

Copy link
Contributor

@AndrewJakubowicz AndrewJakubowicz left a comment

Choose a reason for hiding this comment

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

Confirm bug is resolved.
LGTM!

@jmif jmif merged commit ce6a534 into master Jul 20, 2019
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.

2 participants