Conversation
…o default styles defined
…gical padding shorthands
… and support border-spacing
…d re-applying width bounds
…honour authored direction
… and drop dead weight
jsamr
left a comment
jsamr
left a comment
There was a problem hiding this comment.
Need to review more, it's quite big. Will review a second time by commit
|
|
||
| function parseSpacing(value: string, node: TNode): BorderSpacing | null { | ||
| const parts = value.split(/\s+/); | ||
| if (parts.length < 1 || parts.length > 2) return null; |
There was a problem hiding this comment.
Which case does parts.length > 2 cover?
| const ZERO: BorderSpacing = { horizontal: 0, vertical: 0 }; | ||
|
|
||
| const LENGTH_REGEX = /^(\d*\.?\d+)(px|em|rem|pt|pc|in|cm|mm)?$/; | ||
|
|
||
| /** CSS absolute units, in px. */ | ||
| const ABSOLUTE_SCALES: Record<string, number> = { | ||
| px: 1, | ||
| pt: 96 / 72, | ||
| pc: 16, | ||
| in: 96, | ||
| cm: 96 / 2.54, | ||
| mm: 96 / 25.4 | ||
| }; |
There was a problem hiding this comment.
Nit: I don't think this project has any UPPERCASE const precendents and don't see why it would
| return root; | ||
| } | ||
|
|
||
| function parseSpacing(value: string, node: TNode): BorderSpacing | null { |
There was a problem hiding this comment.
Can we be more specific than "value"? Is this a CSS declaration? Inline style value? Let's use the accurate naming from existing specs.
EDIT: from call site context, it should be inlineStyleValue.
| @@ -0,0 +1,27 @@ | |||
| import { TNode } from '@native-html/render'; | |||
|
|
|||
| export function getInlineStyleValue( | |||
There was a problem hiding this comment.
It's actually more life "get property value in inline style", since "get inline style value" sounds like it would return tnode.attributes.style. Confusing; getPropertyValue(inlineStyle, propertyName) would be more legible.
| if (name === propertyName) { | ||
| value = declaration | ||
| .slice(colonIndex + 1) | ||
| .replace(/\s*!important\s*$/i, '') |
There was a problem hiding this comment.
Since you're tampering with the value, you"re doing more than a "get... ". that warrants a code comment, and perhaps even a JSDoc depiction since this function is exported.
2 participants
I added quite a few new features to the
heuristic-table-plugin:I admit - those were mostly vibe-coded, I was looking at the
ComparisonExampleto compare, how does it perform, briefly checking the code, logic, but not going into too much detailsI also run a few rounds of review-fix for most critical issues, and a few refactor rounds
Not everything is covered (double borders, missing cells), but it was good enough for the project on which I am working, and for which I decided to give it a shot to update it as quick as I can
Here are a couple of iOS screeshots from the example:
And here are android screenshots:
