Skip to content

Conversation

@Alenar
Copy link
Collaborator

@Alenar Alenar commented Nov 19, 2024

Content

This PR reworks the explorer upper part (that contains the aggregator selection, status and refresh interval setter) to make it more easier to use and free some space:

  • Group refresh interval, aggregator selection, aggregator status/epoch setting into a ControlPanel component to make the code more cohesive

  • Replace Aggregator Status and Epoch Setting titles with a Aggregator title that is before the aggregator and refresh interval selectors

  • Remove labels on the aggregator selection and refresh interval setter to free some vertical space

  • Interval setter:

    • Replace play/pause button with a Refresh button
    • Add a Off value to the available options to stop the automatic refresh
    • Add a tool-tip on the available refresh interval options to compensate the removal of the label

    Previous button was ambiguous and made little sense and if we only want to refresh once we had to click on it twice, by regrouping all the interval values in the options and adding a dedicated button the UI should be easier to use.

  • Make the EpochSettings component collapsible the same way as the aggregator status (for aggregators that does not yet support the /status route)

Demo:
Screencast from 2024-11-19 16-25-00.webm

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@Alenar Alenar self-assigned this Nov 19, 2024
@Alenar Alenar changed the title Explorer: Rework aggregator controls Explorer: Easier, more cohesive, aggregator controls Nov 19, 2024
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

github-actions bot commented Nov 19, 2024

Test Results

    4 files  ±0     51 suites  ±0   11m 42s ⏱️ +7s
1 446 tests ±0  1 446 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 657 runs  ±0  1 657 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 5f15458. ± Comparison against base commit 98890a8.

♻️ This comment has been updated with latest results.

@Alenar Alenar temporarily deployed to testing-preview November 19, 2024 15:59 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet November 19, 2024 15:59 — with GitHub Actions Inactive
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

+ Add a 'Off' option in the update interval select to replace the
  'pause'.
…atus

In order to move the after the 'Aggregator' title row.

+ Remove their labels to reclaim some vertical space
…aggregator status

- remove its header
- move its 'raw json' button to its card footer
- make it collapsable
…ta and refresh interval in a `ControlPanel` component
@Alenar Alenar force-pushed the djo/explorer/regroup-aggregator-control-status branch from 2347bee to 5f15458 Compare November 19, 2024 17:52
@Alenar Alenar temporarily deployed to testing-preview November 19, 2024 18:02 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet November 19, 2024 18:02 — with GitHub Actions Inactive
@Alenar Alenar merged commit 348aa6e into main Nov 19, 2024
47 of 48 checks passed
@Alenar Alenar deleted the djo/explorer/regroup-aggregator-control-status branch November 19, 2024 18:05
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