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

Property-attribute sync scheme #95

Merged
merged 10 commits into from
Aug 29, 2021
Merged

Conversation

nizniz187
Copy link
Contributor

Major changes of WComponent including:

  • Refactor default values to attributes object to encompass name, default value, observed, and parser function for easier dynamical parsing.
  • Add getObservedAttributes class function for getting observed attributes array.
  • Add createGettersAndSetters function for dynamically add getters & setters for all observed attributes.

For more details, please refer to the code comments.
To test if this scheme is valid, please use w-checkbox & w-radio only for testing. (Other component will need to be revised to align with the new scheme to work.)

@nizniz187 nizniz187 added the enhancement New feature or request label Aug 24, 2021
@nizniz187 nizniz187 added this to the Architecture milestone Aug 24, 2021
@nizniz187 nizniz187 requested a review from cwpeng August 24, 2021 16:35
@nizniz187
Copy link
Contributor Author

@cwpeng
Please review it first, then I'll fix all the broken components and do the merge.

@cwpeng
Copy link
Member

cwpeng commented Aug 25, 2021

Should we write updating code depending on arguments of attributeChangedCallback, not previous set property?
attributeChangedCallback(name, oldValue, newValue){}

I cannot make setAttribute("checked", false) work after setting it to true.

@nizniz187
Copy link
Contributor Author

True.
Fixed.
This might be the reason why my demo failed yesterday XD

@nizniz187 nizniz187 moved this from Next To Do to In progress in Debut: Version 1.0 - Pizza 🍕 Aug 25, 2021
Copy link
Member

@cwpeng cwpeng left a comment

Choose a reason for hiding this comment

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

OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

2 participants