Skip to content

Conversation

@koddsson
Copy link
Contributor

Add flow declaration file and some minor refactoring.

@koddsson koddsson requested a review from a team May 24, 2019 08:48

connectedCallback() {
if (this.constructed) return
this.constructed = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@muan I just removed this, not sure what this was doing 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

This was preventing the template from being appended every time connectedCallback is triggered since we can't add nodes to the element in constructor.

This can potentially check for existence of .crop-wrapper within the node instead. However we should probably also convert these classes to data attributes and update the CSS accordingly since these classes could conflict with the host app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was preventing the template from being appended every time connectedCallback is triggered since we can't add nodes to the element in constructor.

Cool! I just pushed a commit that checks a WeakMap for the element and returns if it has added the template already.

This can potentially check for existence of .crop-wrapper within the node instead. However we should probably also convert these classes to data attributes and update the CSS accordingly since these classes could conflict with the host app.

Would it be cool if I did this in a follow up?


connectedCallback() {
if (this.constructed) return
this.constructed = true
Copy link
Contributor

Choose a reason for hiding this comment

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

This was preventing the template from being appended every time connectedCallback is triggered since we can't add nodes to the element in constructor.

This can potentially check for existence of .crop-wrapper within the node instead. However we should probably also convert these classes to data attributes and update the CSS accordingly since these classes could conflict with the host app.

target.dispatchEvent(new CustomEvent('image-crop-change', {bubbles: true, detail: result}))
}

export class ImageCropElement extends HTMLElement {
Copy link

Choose a reason for hiding this comment

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

Is the a reason the class is no longer exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's exported as default at the end of this file so I removed this named export.

Store els in constrcutedElements
@koddsson koddsson merged commit 0ae0353 into master Jun 6, 2019
@keithamus keithamus deleted the add-flow-declaration-file branch August 3, 2020 14:55
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.

4 participants