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

refactor(core): workbench #7355

Open
wants to merge 1 commit into
base: canary
Choose a base branch
from
Open

refactor(core): workbench #7355

wants to merge 1 commit into from

Conversation

EYHN
Copy link
Member

@EYHN EYHN commented Jun 27, 2024

Merge the right sidebar logic into the workbench. this can simplify our logic.

Previously we had 3 modules

  • workbench
  • right-sidebar (Control sidebar open&close)
  • multi-tab-sidebar (Control tabs)

Now everything is managed in Workbench.

Behavioral changes

The sidebar button is always visible and can be opened at any time.
If there is no content to display, will be No Selection

CleanShot 2024-06-28 at 14.00.41.png

Elements in the sidebar can now be defined asunmountOnInactive=false. Inactive sidebars are marked with display: none but not unmount, so the ChatPanel can always remain in the DOM and user input will be retained even if the sidebar is closed.

Copy link

graphite-app bot commented Jun 27, 2024

Your org has enabled the Graphite merge queue for merging into canary

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

Copy link
Member Author

EYHN commented Jun 27, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @EYHN and the rest of your teammates on Graphite Graphite

Copy link

nx-cloud bot commented Jun 27, 2024

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 15.87302% with 106 lines in your changes missing coverage. Please review.

Project coverage is 58.24%. Comparing base (d72dbe6) to head (6447019).

Files Patch % Lines
...d/core/src/modules/workbench/view/view-islands.tsx 33.33% 24 Missing ⚠️
...core/src/modules/workbench/view/workbench-root.tsx 12.00% 22 Missing ⚠️
...ontend/core/src/modules/workbench/entities/view.ts 0.00% 15 Missing ⚠️
...d/core/src/modules/workbench/entities/workbench.ts 0.00% 14 Missing ⚠️
...dules/workbench/view/sidebar/sidebar-container.tsx 6.66% 14 Missing ⚠️
...ore/src/modules/workbench/view/route-container.tsx 0.00% 6 Missing ⚠️
...workbench/view/sidebar/sidebar-header-switcher.tsx 14.28% 6 Missing ⚠️
...ontend/core/src/modules/workbench/services/view.ts 0.00% 3 Missing ⚠️
...onent/src/components/resize-panel/resize-panel.tsx 0.00% 1 Missing ⚠️
...core/src/modules/workbench/entities/sidebar-tab.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           canary    #7355   +/-   ##
=======================================
  Coverage   58.23%   58.24%           
=======================================
  Files         856      853    -3     
  Lines       37735    37746   +11     
  Branches     4087     4086    -1     
=======================================
+ Hits        21976    21984    +8     
- Misses      15466    15469    +3     
  Partials      293      293           
Flag Coverage Δ
server-test 77.91% <ø> (ø)
unittest 28.34% <15.87%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EYHN EYHN force-pushed the eyhn/refactor/workbench branch 2 times, most recently from 391b08e to c0d17c4 Compare June 28, 2024 05:56
@github-actions github-actions bot added the docs Improvements or additions to documentation label Jun 28, 2024
@EYHN EYHN marked this pull request as ready for review June 28, 2024 05:57
Copy link
Collaborator

@pengx17 pengx17 left a comment

Choose a reason for hiding this comment

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

can you update the description on the purpose of changes?

import { createNavigableHistory } from '../../../utils/navigable-history';
import type { ViewScope } from '../scopes/view';

export class View extends Entity {
id = this.scope.props.id;

sidebarTabs$ = new LiveData<string[]>([]);
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Why not keep Sidebar entity but manage in view directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I deleted the sidebar, and the sidebar is now managed by the view

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I meant that is it more intuitive to maintain a Sidebar entity for the logic of the sidebar, as view.sidebar?

@EYHN
Copy link
Member Author

EYHN commented Jun 28, 2024

can you update the description on the purpose of changes?

Simplified the logic of the sidebar, which was a bit over-designed before.

And now that the sidebar is included in the workbench, their states can also be saved and persisted together.

@EYHN EYHN force-pushed the eyhn/refactor/workbench branch 2 times, most recently from da08b91 to 6ae500b Compare July 1, 2024 05:40
@EYHN EYHN force-pushed the eyhn/refactor/workbench branch from 6ae500b to 6447019 Compare July 1, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app:core docs Improvements or additions to documentation mod:component
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

3 participants