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

(dashboard) Info pane refactor #751

Merged
merged 3 commits into from
May 6, 2019
Merged

(dashboard) Info pane refactor #751

merged 3 commits into from
May 6, 2019

Conversation

eysi09
Copy link
Collaborator

@eysi09 eysi09 commented May 3, 2019

In this PR:

  • All task nodes are clickable and have an info pane.
  • Nodes with test or task results also get a refresh button. We probably want to style it a little better (cc @benstov).
  • LoadWrapper component is removed. Rendering logic is much clearer without it.
  • Previous info pane gets refactored into generic containers and components.
  • Use useReducer instead of useState in the useApi hook. It's a more idiomatic pattern for nested data structures like the Store blob.
  • Some style changes to the info pane. Keys are now bold and the Output: text has been removed. See screenshot below. This is of course debatable and easy to revert.

Screenshot 2019-05-03 at 20 03 34

@eysi09 eysi09 force-pushed the info-pane-refactor branch 2 times, most recently from 01210ab to 5da3120 Compare May 3, 2019 18:50
@eysi09
Copy link
Collaborator Author

eysi09 commented May 3, 2019

Going forward, it might be nice to pull the load functions out of the useApi hook. If only to prevent it from growing linearly with the number of load calls. That would also make the loaders more flexible. On the flip side, we'd need to pass the store and dispatch function to all of them. So every container would need to get the store and dispatch function from the data context and then do something like loadLogs(store, dispatch, ...otherArgs).

Alternatively we could do away with the load functions and just pass the dispatch to the container components. They could then do something like: dispatch({ type: loadLogs }). That would reduce some code (pun kind of intended) but it feels a little to loose though.

@eysi09 eysi09 requested review from thsig and benstov and removed request for thsig May 5, 2019 18:13
@benstov benstov changed the title Info pane refactor (dashboard) Info pane refactor May 6, 2019
eysi09 added 3 commits May 6, 2019 09:52
As a side effect, all nodes now have a info pane. Also added a refresh
button to the pane and made minor style changes.
It makes the fetch logic more readable and deep merges easier.
@eysi09 eysi09 force-pushed the info-pane-refactor branch from 754f859 to 8bc67d0 Compare May 6, 2019 07:55
@eysi09 eysi09 merged commit 9ec4e7e into master May 6, 2019
@eysi09 eysi09 deleted the info-pane-refactor branch May 6, 2019 08:07
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.

2 participants