search

shurcooL / Go-Package-Store

An app that displays updates for the Go packages in your GOPATH.
MIT License
900 stars 29 forks source link

Fix rendering after Vecty API changes. #88

Closed dmitshur closed 6 years ago

dmitshur commented 6 years ago

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.

dmitshur commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 6 years ago

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 commented 6 years ago

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

dmitshur commented 6 years ago

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!