Skip to content
This repository was archived by the owner on Dec 18, 2024. It is now read-only.

fix(plunker): make calls to plunker synchronous to avoid popup block #83

Merged
merged 3 commits into from
Jan 17, 2017

Conversation

andrewseguin
Copy link
Collaborator

No description provided.

Copy link
Contributor

@tinayuangao tinayuangao left a comment

Choose a reason for hiding this comment

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

LGTM

* The button becomes disabled if the user hovers over the button before the plunker form
* is created. After the form is created, the button becomes enabled again.
*/
isDisabled: boolean = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make it disabled by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initially I had it disabled by default and only enabled when the form was created. However, this caused a bit of a flicker on the button where it goes from disabled -> enabled right after the page loads.

With the current implementation, it always appears active to the user except in the small chance they manage to mouse over it to interact with it. In which case, it disables itself and shows a tooltip saying that we building the form.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, one minor comment


@Input()
set example(example: string) {
this.exampleData = new ExampleData(example);
const exampleData = new ExampleData(example);
this.plunkerWriter.constructPlunkerForm(exampleData).then(plunkerForm => {
Copy link
Member

Choose a reason for hiding this comment

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

Add something like "This usually happens extremely quickly, but we handle the case of the plunker not yet being ready for people will poor network connections or slow devices."

@andrewseguin
Copy link
Collaborator Author

Added a comment about the speed of creating the form

@jelbourn jelbourn merged commit a8f83de into angular:master Jan 17, 2017
@andrewseguin andrewseguin deleted the make-plunker-link-synchronous branch February 10, 2017 00:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants