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

Fix rendering after Vecty API changes. #88

Merged
merged 1 commit into from Oct 12, 2017

Conversation

Projects
None yet
3 participants
@dmitshur
Member

dmitshur commented Oct 10, 2017

The goal of this PR is to make the smallest needed change to have correct rendering behavior, that's it. It should be as small as possible. It's a subset of #86.

See individual commit messages for more details.

component: Mark properties with `vecty:"prop"` tag.
This is needed to have correct rendering behavior after the breaking
API change of gopherjs/vecty#130. All of these field are properties
that are set by external components, they are not internal state of
the component itself.

See gopherjs/vecty#149 (comment):

>	anywhere a Component is instantiated inside Component.Render(),
>	with fields set from the parent, those fields need to be tagged
>	with `vecty:"prop"`.

And gopherjs/vecty#130 (comment):

>	Fields that are updated from the parent need to be tagged:
>
>		type InnerComponent struct {
>		  vecty.Core
>		  ParentSet bool `vecty:"prop"`
>		  localState bool
>		}
>
>		func (c *InnerComponent) Render() *vecty.HTML {
>		  c.localState = !c.localState
>		  return elem.Div(
>		    vecty.If(c.ParentSet, elem.Span(`true updated from parent`)),
>		    vecty.If(c.localState, elem.Span(`true updated locally`)),
>		  )
>		}
>
>		type OuterComponent struct {
>		  vecty.Core
>		  flip bool
>		}
>
>		func (c *OuterComponent) Render() *vecty.HTML {
>		  c.flip = !c.flip
>		  return elem.Div(
>		    &InnerComponet{ParentSet: c.flip},
>		  )
>		}

Helps gopherjs/vecty#149.
Follows e7b1bc5. /cc @slimsag @pdf
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 10, 2017

Member

Unfortunately, this change doesn't seem to be enough to have correct rendering. I'm still seeing a bug, but it's different than before. It's reproducible using the usual mock.html page.

How it should look like after last render:

image

image

How it actually looks:

image

image

Note that the "changes-list" class is missing. It happens only after the final render, when all elements are shifted by one because the "checking for updates..." heading goes away.

Any ideas what's causing the incorrect rendering behavior @pdf or @slimsag?

Member

dmitshur commented Oct 10, 2017

Unfortunately, this change doesn't seem to be enough to have correct rendering. I'm still seeing a bug, but it's different than before. It's reproducible using the usual mock.html page.

How it should look like after last render:

image

image

How it actually looks:

image

image

Note that the "changes-list" class is missing. It happens only after the final render, when all elements are shifted by one because the "checking for updates..." heading goes away.

Any ideas what's causing the incorrect rendering behavior @pdf or @slimsag?

@pdf

This comment has been minimized.

Show comment
Hide comment
@pdf

pdf Oct 10, 2017

Not off the top of my head, no. It's really hard to do any debugging without source maps though. I guess it must have something to do with the replacement of the ul element in the previous render with a div, but I'm not sure why that would cause any problem in this particular case.

pdf commented Oct 10, 2017

Not off the top of my head, no. It's really hard to do any debugging without source maps though. I guess it must have something to do with the replacement of the ul element in the previous render with a div, but I'm not sure why that would cause any problem in this particular case.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 11, 2017

Member

Thanks.

I'll try to investigate more and see how hard it is to make a smaller repro. The changes you did in #86 didn't have this issue. But I'm interested in finding out what's causing it, in case it's a bug in rendering which would be worth fixing rather than working around.

Member

dmitshur commented Oct 11, 2017

Thanks.

I'll try to investigate more and see how hard it is to make a smaller repro. The changes you did in #86 didn't have this issue. But I'm interested in finding out what's causing it, in case it's a bug in rendering which would be worth fixing rather than working around.

@pdf

This comment has been minimized.

Show comment
Hide comment
@pdf

pdf Oct 11, 2017

It must be a bug, so I would certainly like to fix it. In #86, that element would never get replaced, which I suspect is why it wasn't evident there. I'll also try to find a repro, but probably not until the weekend.

pdf commented Oct 11, 2017

It must be a bug, so I would certainly like to fix it. In #86, that element would never get replaced, which I suspect is why it wasn't evident there. I'll also try to find a repro, but probably not until the weekend.

@pdf

This comment has been minimized.

Show comment
Hide comment
@pdf

pdf Oct 11, 2017

Had a little time this evening to test and was able to reproduce the problem easily enough. Will be fixed by merging gopherjs/vecty#154. The bug is triggered by replacing an element with a new element of a different type, when both elements share properties with matching keys and values.

pdf commented Oct 11, 2017

Had a little time this evening to test and was able to reproduce the problem easily enough. Will be fixed by merging gopherjs/vecty#154. The bug is triggered by replacing an element with a new element of a different type, when both elements share properties with matching keys and values.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Oct 12, 2017

Contributor

Major thanks pdf for investigating / fixing this so quickly! I've just merged his PR gopherjs/vecty#154 above, so I think this should be fixed now :)

Contributor

slimsag commented Oct 12, 2017

Major thanks pdf for investigating / fixing this so quickly! I've just merged his PR gopherjs/vecty#154 above, so I think this should be fixed now :)

slimsag added a commit to gopherjs/vecty that referenced this pull request Oct 12, 2017

Merge pull request #154 from pdf/keyed_children
When an element was replaced by an element of conflicting type, but
shared properties, attributes, datasets, etc, with equal values, those
properties were not applied to the new element.

Fixes shurcooL/Go-Package-Store#88 (comment)

(This empty commit adds the missing commit message for the prior commit,
8db3466, which I accidentally typo'd.
see #154 (comment))
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 12, 2017

Member

It is indeed. I'm not seeing any other issues, so I can proceed with merging this PR now. Thank you again @pdf and @slimsag!

Member

dmitshur commented Oct 12, 2017

It is indeed. I'm not seeing any other issues, so I can proceed with merging this PR now. Thank you again @pdf and @slimsag!

@dmitshur dmitshur merged commit d8599c0 into master Oct 12, 2017

4 checks passed

ci/gopherci/pr Found no issues \ʕ◔ϖ◔ʔ/
Details
ci/gopherci/push Found no issues \ʕ◔ϖ◔ʔ/
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@dmitshur dmitshur deleted the fix-rendering-after-130 branch Oct 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment