-
Notifications
You must be signed in to change notification settings - Fork 64
[REF] side_panel: split CF/DV panel into main and editor panel #7183
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi 👋
Here are some comments to help speed up the review process.
describe("Conditional format list", () => { | ||
let sheetId: UID; | ||
describe("Conditional formatting list panel", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn’t remove any test cases; I only reordered them based on the preferred environment and titles. 😑
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, I used a store to separate business logic from UI logic, and it was also needed to handle the undo operation.
// TODO: add manage rule in (sleection/sheet) feature like excel does | ||
const rules = env.model.getters.getConditionalFormats(sheetId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋
maybe the ConditionalFormattingPanel
and DataValidationPanel
might no longer be needed, since you’re already switching between panels directly through the side panel.
Also found a bug:
- Add enough cfs so the side panel shows a scrollbar
- Drag the first item to the bottom
- The panel doesn’t scroll down automatically
src/components/side_panel/conditional_formatting/conditional_formatting.xml
Outdated
Show resolved
Hide resolved
38253d8
to
593d218
Compare
I removed the list panel because it has become useless. |
Before this commit: - The data validation side panel and its editor were combined in a single panel. - This prevented opening the editor directly. - The behavior was inconsistent with pivot and other panels, and the structure was less modular. After this commit: - The data validation panel and the editor are now separate panels. - Panels can be opened or replaced using `openSidePanel` and `replaceSidePanel`. - Any panel can now be opened directly when needed. Task: 4982596
593d218
to
b5c5b44
Compare
Before this commit: - The cf side panel handled both list and editor in a single panel. - The logic of the main panel and the editor was mixed, preventing direct access to the editor. - Business and UI logic for the editor were coupled. After this commit: - Use a store to manage the state of the editor panel. - Make the main panel and editor panel independent. - Change undo behavior in the editor to redirect back to the main panel. - Refactor related test cases. Task: 4982596
Before this commit: - The conditional formatting editor used the class name o-cf-ruleEditor. - The rule editor itself was named o-cf-editor, which was inconsistent and confusing. After this commit: - CSS classes are renamed to match their actual purpose, improving consistency and readability. Task: 4982596
b5c5b44
to
1cdf54f
Compare
Description:
Current behavior before PR:
Desired behavior after PR is merged:
Task: 4982596
review checklist