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

UI cell style #118

Open
wants to merge 55 commits into
base: main
Choose a base branch
from
Open

UI cell style #118

wants to merge 55 commits into from

Conversation

rafinutshaw
Copy link
Contributor

@rafinutshaw rafinutshaw commented Apr 14, 2024

Main features

  • Add styling info in the interface with methods to change styles "setCss", "setFormatter", "cleatCss" and "clearFormatter". I made the css and formatter seperate, I believe this is a better usercase. and it also seperates our concerns
  • Add relevent tests

Additional work

  • Remove keys and make use of index to communicate between ui and core
  • rename ui to view
  • move root utils to merge it with utils/helpers.ts
  • rename type Position to KeyInfo and Coordinate to IndexInfo

Didnt do

  • custom formatter

@rafinutshaw rafinutshaw marked this pull request as draft April 14, 2024 17:32
@Yousif-FJ Yousif-FJ linked an issue Apr 15, 2024 that may be closed by this pull request
@julkunensuvi julkunensuvi linked an issue Apr 15, 2024 that may be closed by this pull request
@rafinutshaw rafinutshaw marked this pull request as ready for review April 29, 2024 02:09
Comment on lines 108 to 116
it("should create an empty cell with styling", () => {
const b2 = sheet.getCellInfoAt(1, 1)!;
sheet.setCellStyle(
b2.position!.columnKey!,
b2.position!.rowKey!,
new CellStyle(new Map([["width", "50px"]])),
);
sheet.setCellCss(1, 1, new Map<string, string>([["width", "50px;"]]));

sheet.setCellAt(1, 1, "");

// Clearing the style should result in the cell being deleted.
sheet.setCellStyle(b2!.position.columnKey!, b2!.position.rowKey!, null);
sheet.setCellCss(1, 1, new Map<string, string>());
expect(sheet.getCellInfoAt(1, 1)).toBeNull();
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is failing. maybe I did something wrong something. I am a bit tired as well, It would be nice if someone can get me insights on why this maybe happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

deleteCellIfUnused only checks for the existence of a style. SetCellCss calls CellStyle::clearCss, but never cleans up empty CellStyle objects assigned to cells. I'd suggest using the existing clearCellStyle method, maybe along with a method similar to deleteCellIfUnused that checks if a cell-specific style is still relevant after a change. The code should make sure that no unused or irrelevant (i.e., empty or not differing from group/sheet default style) styling objects are stored.

Copy link
Contributor

@Fluglow Fluglow left a comment

Choose a reason for hiding this comment

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

Didn't have time to look at the tests yet.

src/core/event/eventType.ts Outdated Show resolved Hide resolved
src/core/structure/cellStyle.ts Outdated Show resolved Hide resolved
src/core/structure/sheet.types.ts Outdated Show resolved Hide resolved
src/core/event/events.types.ts Outdated Show resolved Hide resolved
src/core/structure/sheet.ts Outdated Show resolved Hide resolved
src/core/structure/sheet.ts Outdated Show resolved Hide resolved
src/core/structure/sheet.types.ts Outdated Show resolved Hide resolved
Comment on lines +18 to +22
export enum GroupTypes {
Column = 1,
Row,
}
export type GroupType = GroupTypes;
Copy link
Contributor

Choose a reason for hiding this comment

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

These should not exist

src/main.ts Outdated Show resolved Hide resolved
src/utils/common.types.ts Outdated Show resolved Hide resolved
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.

Handle cells styling in UI Implement event protocol for styling
2 participants