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

Support docker rename events #1332

Merged
merged 3 commits into from
Apr 20, 2016
Merged

Support docker rename events #1332

merged 3 commits into from
Apr 20, 2016

Conversation

paulbellamy
Copy link
Contributor

Fixes #1091

@foot, doesn't propagate through to the canvas, just the details panel.

@tomwilkie
Copy link
Contributor

Backend LGTM, although am a little worries that it doesn't come through to the canvas. Is the backend sending the right data to the frontend @paulbellamy ?

@paulbellamy
Copy link
Contributor Author

paulbellamy commented Apr 19, 2016

It shows the right data in the details panel, and when you refresh, but it doesn't in-place-update the canvas

Edit: Which makes me suspect the JS.

@tomwilkie
Copy link
Contributor

Details panel is polled though, whereas the canvas is a push ws from the app.

@paulbellamy
Copy link
Contributor Author

The websocket is sending out the updated node label.

@foot
Copy link
Contributor

foot commented Apr 19, 2016

Cool will get the canvas behaving.

@foot foot self-assigned this Apr 19, 2016
@foot foot assigned davkal and unassigned foot Apr 19, 2016
@foot
Copy link
Contributor

foot commented Apr 19, 2016

@davkal rfr not sure if this is the best way to do it?

@@ -356,7 +353,7 @@ export default class NodesChart extends React.Component {

// inject metrics and save coordinates for restore
const layoutNodes = graph.nodes
.mergeDeep(nodeMetrics)
.mergeDeep(stateNodes)

This comment was marked as abuse.

@davkal
Copy link
Contributor

davkal commented Apr 19, 2016

Looks like nodes-layout:copyLayoutProperties() overwrites the nodes with data from the node cache. 😞
Re-applying the stateNodes after the layout run works, but only after the node locations has been backed up.

I added #1353 as a techdebt ticket to rewrite that part in nodes-layout.

@davkal davkal assigned foot and unassigned davkal Apr 19, 2016
@foot
Copy link
Contributor

foot commented Apr 20, 2016

FE is ready to merge @paulbellamy

@foot foot assigned paulbellamy and unassigned foot Apr 20, 2016
@paulbellamy paulbellamy merged commit 5cedfad into master Apr 20, 2016
@paulbellamy paulbellamy deleted the 1091-docker-rename branch April 20, 2016 08:45
@rade
Copy link
Member

rade commented Apr 20, 2016

@foot What is the intended f/e behaviour? I've played with this and renaming does not update the UI. Even switching between views has no effect. However, the UI does update when hitting the 'redraw' button.

@foot
Copy link
Contributor

foot commented Apr 20, 2016

Intended behaviour is immediate instantaneous UI updates! So fast it changes before you can alt-tab back from the terminal and check that it changed!

Which is a bum, clear cache? I'll give it a quick test in Firefox now.

#1354 Should help w/ the caching issues if it turns out to be that.

@rade
Copy link
Member

rade commented Apr 20, 2016

clear cache

That was it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants