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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bugfix/real incremental diff based rendering #411

Merged
merged 22 commits into from
May 1, 2018
Merged

Bugfix/real incremental diff based rendering #411

merged 22 commits into from
May 1, 2018

Conversation

AndyOGo
Copy link

@AndyOGo AndyOGo commented Apr 18, 2018 •
edited

Fixes #408
fixes #416

Changes proposed in this pull request:

  • installs nanomorph We morph by ourselves
  • fixes DOM subtree replacements cause loos of intrinsic <input /> states like focus

Type of change

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

How Has This Been Tested?

Needs tests in every browser.

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 added this to the React Integration milestone Apr 18, 2018
@AndyOGo AndyOGo self-assigned this Apr 18, 2018
This was referenced Apr 18, 2018
Merged
Open
Open
Closed
@AndyOGo
Copy link
Author

AndyOGo commented Apr 30, 2018

@LucaMele
@TheDadi
the stroke is broken by enabling incremental rendering 馃槥

morph-stroke-broken

@AndyOGo
Copy link
Author

AndyOGo commented Apr 30, 2018

stroke fixed

@AndyOGo
Copy link
Author

AndyOGo commented Apr 30, 2018

Namespaced attributes are being tracked in a separate branch #416

LucaMele
LucaMele suggested changes Apr 30, 2018
View reviewed changes
Copy link
Contributor

@LucaMele LucaMele left a comment

Choose a reason for hiding this comment

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

Firefix and IE11 broken therefore all the browser that uses the custom element polyfill are broken

LucaMele
LucaMele reviewed Apr 30, 2018
View reviewed changes
src/js/abstract/base-component.js Outdated

if (ENV !== PROD) {
lifecycleLogger(this.logLifecycle)(`willRenderCallback -> ${this.nodeName}#${this._id} <- initial: ${!initial}`);
lifecycleLogger(this.logLifecycle)(`willRenderCallback -> ${this.nodeName}#${this._id} <- initial: ${!notInitial}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

not notInitial is more readable?

LucaMele
LucaMele reviewed Apr 30, 2018
View reviewed changes
src/js/abstract/base-component.js Outdated
* Monkey patch `childNodes` API to re-rendering.
*/
get childNodes() {
if (this._morphing || !this._hasTemplate || !this._hasRendered) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe isMorphing ?

LucaMele
LucaMele reviewed Apr 30, 2018
View reviewed changes
src/js/abstract/base-component.js Outdated
*/
// @todo: ideally this code is only attached during morphing phase
function isSameNodeOnce(node) {
node.isSameNode = isSameNodeStopMorph;
Copy link
Contributor

Choose a reason for hiding this comment

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

node.isSameNode = () => true;

AndyOGo
AndyOGo commented Apr 30, 2018
View reviewed changes
src/js/abstract/base-component.js Outdated
/**
* Monkey patch `childNodes` API to re-rendering.
*/
get childNodes() {
Copy link
Author

Choose a reason for hiding this comment

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

@LucaMele
This block breaks the Web-Component polyfill, they also patch querySelector by accessing childNodes.
The polyfill is really doing allot of stuff, patching various DOM API's and I think we should get aware of it. Otherwise get into big trouble...

@AndyOGo AndyOGo force-pushed the bugfix/real-incremental-diff-based-rendering branch from 2f1e347 to 28981a5 Compare April 30, 2018 15:41
AndyOGo
AndyOGo commented Apr 30, 2018
View reviewed changes
src/js/abstract/component-morph.js
@@ -0,0 +1,173 @@
import morph from './morph';
Copy link
Author

@AndyOGo AndyOGo Apr 30, 2018 •
edited

Choose a reason for hiding this comment

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

we may will create it's own npm package for morphin in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a todo in the file directly and also WHY we did copy/modify the nanomorph one

@AndyOGo
Copy link
Author

AndyOGo commented Apr 30, 2018

@LucaMele
All fixed, Firefox works, SVG icon with xlink works, etc.

@AndyOGo AndyOGo requested a review from LucaMele April 30, 2018 15:51
@AndyOGo
Copy link
Author

AndyOGo commented Apr 30, 2018

Successfully tested in Edge 15 and 16 too

LucaMele
LucaMele approved these changes May 1, 2018
View reviewed changes
Copy link
Contributor

@LucaMele LucaMele left a comment

Choose a reason for hiding this comment

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

Tested attr change on
Firefox, Chrome and Safari IOS. All good.

and runs on IE11 :)

@LucaMele
Copy link
Contributor

LucaMele commented May 1, 2018 •
edited

@AndyOGo ur a coder GOD

@AndyOGo
Copy link
Author

AndyOGo commented May 1, 2018

@LucaMele
Thank you very much courageous testing:)

hehe, thanks for your very friendly commendation 馃槂

@AndyOGo AndyOGo merged commit ec5bcdc into develop May 1, 2018
@AndyOGo AndyOGo deleted the bugfix/real-incremental-diff-based-rendering branch May 1, 2018 08:57
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

@pavestru pavestru Awaiting requested review from pavestru

Assignees

@AndyOGo AndyOGo

Labels
Bug High-prio
Projects
None yet
Milestone
React Integration
Development

Successfully merging this pull request may close these issues.

Namespaced attributes aren't updated incrementally Rerender loses focus of input
2 participants
@AndyOGo @LucaMele