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.

Sign up for GitHub

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

Jump to bottom

Bugfix/fix props vs attrs #356

Merged
merged 32 commits into from
Mar 28, 2018
Merged

Bugfix/fix props vs attrs #356

merged 32 commits into from
Mar 28, 2018

Conversation

AndyOGo
Copy link

@AndyOGo AndyOGo commented Mar 21, 2018 •
edited
Loading

Fixes #321 and foundations for #39 .

Changes proposed in this pull request:

  • Adds DOM property getters and setters for related HTML attributes
  • Properly checks for Arrays in templates

The idea is that first-class props can be set by DOM properties. In case of HTML's setAttribute is used, we parse the attributes value once for JSON and store the result as first-class DOM property. This way it will be much easier to bridge with frameworks like React or Angular and getAttribute/s won't be needed any more, just simple prop access.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

How Has This Been Tested?

Locally

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@AndyOGo AndyOGo self-assigned this Mar 21, 2018
@AndyOGo AndyOGo requested review from LucaMele and TheDadi March 21, 2018 14:51
LucaMele
LucaMele reviewed Mar 22, 2018
View reviewed changes
src/components/a-icon/_preview.html Outdated
<axa-icon icon="logo-AXA" classes="a-icon"></axa-icon>
<axa-icon id="axa-icon-test" icon="logo-AXA" classes="a-icon"></axa-icon>

<script>
Copy link
Contributor

Choose a reason for hiding this comment

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

ahahahaha like this!

@AndyOGo AndyOGo force-pushed the bugfix/fix-props-vs-attrs branch 2 times, most recently from a98429e to 729edd9 Compare March 23, 2018 14:46
@AndyOGo
Copy link
Author

AndyOGo commented Mar 26, 2018 •
edited
Loading

BROKEN right now by an infinite loop

%cWeb Component %cAXA-HEADER-MOBILE%c#2 has an error while loading its template:
RangeError: Maximum call stack size exceeded

Stack Trace: RangeError: Maximum call stack size exceeded
    at HTMLElement._makeContextReady (http://localhost:3000/components/m-header-mobile/index.js:1385:16)
    at HTMLElement.connectedCallback (http://localhost:3000/components/m-header-mobile/index.js:1134:14)
    at HTMLElement.connectedCallback (http://localhost:3000/components/m-header-mobile/index.js:2033:127)
    at HTMLElement.didRenderCallback (http://localhost:3000/components/m-header-mobile/index.js:2057:14)
    at HTMLElement.render (http://localhost:3000/components/m-header-mobile/index.js:1284:14)
    at HTMLElement.connectedCallback (http://localhost:3000/components/m-header-mobile/index.js:1131:12)
    at HTMLElement.connectedCallback (http://localhost:3000/components/m-header-mobile/index.js:2033:127)
    at HTMLElement.didRenderCallback (http://localhost:3000/components/m-header-mobile/index.js:2057:14)
    at HTMLElement.render (http://localhost:3000/components/m-header-mobile/index.js:1284:14)
    at HTMLElement.connectedCallback (http://localhost:3000/components/m-header-mobile/index.js:1131:12)

@AndyOGo AndyOGo force-pushed the bugfix/fix-props-vs-attrs branch 6 times, most recently from ad00df1 to 5743fed Compare March 28, 2018 09:25
AndyOGo
AndyOGo commented Mar 28, 2018
View reviewed changes
src/js/abstract/base-component.js

const key = camelize(name);

this[key] = toProp(newValue);
Copy link
Author

Choose a reason for hiding this comment

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

@LucaMele
I would be keen to know, how removal is detected. Guess just an empty string ''.
May we should add something like prop-types but in separate PR, as soon as it becomes an issue. And I would realy like to have automatic tests for this...

Copy link
Contributor

Choose a reason for hiding this comment

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

me too... me too

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit dangerous to be honest.. What if we override a DOM property like style or so on?

Copy link
Author

Choose a reason for hiding this comment

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

Absolutely, may we add a blacklist for ['style', className, id, etc]which throws?
Or I could a check if the property exists already, then throw 😇

Copy link
Contributor

Choose a reason for hiding this comment

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

yes better with check + throw

@AndyOGo AndyOGo mentioned this pull request Mar 28, 2018
Closed
LucaMele
LucaMele approved these changes Mar 28, 2018
View reviewed changes
src/js/abstract/base-component.js
@@ -182,7 +246,7 @@ export default class BaseComponent extends HTMLElement {
});
}

const items = template(getAttributes(this), this.childrenFragment);
const items = template(this._props, this.childrenFragment);
Copy link
Contributor

Choose a reason for hiding this comment

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

i have a mixed feeling trying to reactify standard web components. I like this solution, dont get me wrong, but im just a bit scared :) U can ignore this comment :)

src/js/abstract/base-component.js

const key = camelize(name);

this[key] = toProp(newValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit dangerous to be honest.. What if we override a DOM property like style or so on?

src/js/abstract/base-component.js
if (this.hasAttribute(attr)) {
const value = getAttribute(this, attr);

this[key] = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit dangerous to be honest.. What if we override a DOM property like style or so on?

Copy link
Contributor

Choose a reason for hiding this comment

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

i understand we need it for react thought just want to be sure

Copy link
Author

Choose a reason for hiding this comment

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

I agree. We need to document it and could check if a prop exists already, if so throw

@AndyOGo AndyOGo force-pushed the bugfix/fix-props-vs-attrs branch from c97cf61 to 2f5e197 Compare March 28, 2018 13:57
@AndyOGo AndyOGo requested a review from LucaMele March 28, 2018 14:28
@LucaMele
Copy link
Contributor

hope that super[key] works on all browsers...

@AndyOGo
Copy link
Author

AndyOGo commented Mar 28, 2018

@LucaMele
I hope too. At least I did this trick already for innerHTML, appendChild and stuff like that

@LucaMele
Copy link
Contributor

OK then it should work

@AndyOGo AndyOGo merged commit 60f3a58 into develop Mar 28, 2018
@AndyOGo AndyOGo deleted the bugfix/fix-props-vs-attrs branch March 28, 2018 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucaMele LucaMele LucaMele approved these changes

@TheDadi TheDadi Awaiting requested review from TheDadi

Assignees

@AndyOGo AndyOGo

Labels
Bug High-prio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTML Attributes VS DOM properties
2 participants
@AndyOGo @LucaMele