Changed the type of cell style from string to CellStyle. #82
Conversation
tbouffard
left a comment
There was a problem hiding this comment.
Sorry for the late review, I had to do extra checks to confirm that we broke an existing behavior with the new implementation.
I totally approve the move from string to object to manage the style. This also makes things easier when configuring a cell.
| // Parses each key, value pair into the existing style | ||
| for (const tmp of pairs) { | ||
| const pos = tmp.indexOf('='); | ||
| getCellStyle(cellStyle: CellStyle, defaultStyle: CellStateStyle) { |
There was a problem hiding this comment.
❓ can we add the return type in the method signature for consistency with other methods?
There was a problem hiding this comment.
I've been removing the return types in the method signature when IDEs didn't have problem figuring out the actual type.
But, as you said, getCellStyle didn't have a clear return type, so I added below for IDEs to correctly figure it out.
let style: CellStateStyle;There was a problem hiding this comment.
ok, thanks for the information.
| } else { | ||
| (style[key] as any) = value; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
We are loosing something in the new implementation.
Previously, it was possible to provide several style names according to this implementation. It wasn't explicit in the function documentation. This allowed merging various styles.
By convention, the style names were generally put at the beginning of the string, for instance style1;style2;style3;key1=value1;key2=value2;... but this was not mandatory (the extra style names can be put anywhere in the string).
Clearly, we miss unit tests here that would have exhibit the existing behavior 👿 .
This is something I rely on today with mxgraph and that will disappear with the new implementation.
For the record, here is how it was implemented in mxgraph@4.2.2: https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/view/mxStylesheet.js#L251.
Could we change the baseStyleName to make it a string or an array of strings and update the implementation of the getCellStyle to support it?
There was a problem hiding this comment.
Oh, I didn't know it was possible to have multiple style names.
Making it an array of strings and having a left-to-right merge rule seems good.
I'll work on it as soon as possible.
Thanks!
There was a problem hiding this comment.
So, I renamedbaseStyleName to baseStyleNames and changed it to an array of strings.
Accordingly, passing [] insteand of null clears the default styles.
Hope it works now!
tbouffard
left a comment
There was a problem hiding this comment.
Sorry for the late review, that's look good.
Thanks for this great improvements
| // Parses each key, value pair into the existing style | ||
| for (const tmp of pairs) { | ||
| const pos = tmp.indexOf('='); | ||
| getCellStyle(cellStyle: CellStyle, defaultStyle: CellStateStyle) { |
There was a problem hiding this comment.
ok, thanks for the information.
Summary
Previously when inserting a vertex or an edge into a graph, a string-typed value was needed to style it.
This has brought unnecessary parsing and concating string operations, and also hindered code readability with no syntax highlighting to support it.
Now an object is used instead to replace the string, so that the previous example becomes:
A new type called
CellStyleis introduced to represent the style object, and it is almost identical toCellStateStyle.I've added a new property named
baseStyleNamewhich replaces the previousstylename, so that,style: 'top'becomes,
In order to cover the case when the style starts with a semicolon(without any default style), a
nullcan be assigned tobaseStyleNameto explicitly unset it.I've updated all the examples accordingly, but I'm not 100% sure that everything works.
I'll have to work on the examples one by one after this PR gets merged.
Thanks!
Description for the changelog
Changed the type of cell style from string to CellStyle.
Other info