refactor: alignment to use string literals instead of magic numbers#42
refactor: alignment to use string literals instead of magic numbers#42dreyfus92 wants to merge 5 commits into
Conversation
commit: |
| o += 4; | ||
|
|
||
| view.setUint32(o, (l.alignX ?? 0) | ((l.alignY ?? 0) << 8), true); | ||
| let alignX = l.alignX === "right" ? 1 : l.alignX === "center" ? 2 : 0; |
There was a problem hiding this comment.
hmm, think we might benefit from a more generalized approach to string > enum conversion? like a typed resolveInt(property, value) util?
There was a problem hiding this comment.
Agree. We might even be able to use typescript enums? If so, it would be the first legitimate use-case I've encountered ever (although probably not).
In fairness, there is some of this one-off ternary stuff already in the codebase with "ttb", "ltr", etc.. so not sure it should be a blocker https://github.com/bombshell-dev/clayterm/blob/refactor-alignment/ops.ts#L113
Perhaps we should come up with a const handling strategy and do it all in one go?
natemoo-re
left a comment
natemoo-re
left a comment
There was a problem hiding this comment.
Love this from a DX perspective! Think we'll need to discuss how far we want to take the idea of papering over clay's lower-level layout concepts for a flexbox-friendly audience.
cowboyd
left a comment
cowboyd
left a comment
There was a problem hiding this comment.
@dreyfus92 Nice one! Thanks.
One thing to note: specs/renderer-spec.md needs to be updated.
§12.2 referecences this transition and should now be updated to reflect that it's done. Two spots:
Lines 604–606 — the parenthetical still says alignment is numeric:
Lines 620–622 — drop the trailing sentence about the planned transition:
Happy to do this if you allow maintainers access to the fork :)
| o += 4; | ||
|
|
||
| view.setUint32(o, (l.alignX ?? 0) | ((l.alignY ?? 0) << 8), true); | ||
| let alignX = l.alignX === "right" ? 1 : l.alignX === "center" ? 2 : 0; |
There was a problem hiding this comment.
Agree. We might even be able to use typescript enums? If so, it would be the first legitimate use-case I've encountered ever (although probably not).
In fairness, there is some of this one-off ternary stuff already in the codebase with "ttb", "ltr", etc.. so not sure it should be a blocker https://github.com/bombshell-dev/clayterm/blob/refactor-alignment/ops.ts#L113
Perhaps we should come up with a const handling strategy and do it all in one go?
dreyfus92
force-pushed
the
refactor-alignment
branch
from
1571dde to
fbb9762
Compare
Merging this PR will degrade performance by 54.46%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
cowboyd
left a comment
cowboyd
left a comment
There was a problem hiding this comment.
👍🏻 looks good, thank you!
|
@dreyfus92 looks like the only thing missing is signed commits |
dreyfus92
force-pushed
the
refactor-alignment
branch
from
8157616 to
764dd5a
Compare
|
Size Increased — +0.3 KB 107.8 KB unpacked |
3 participants
closes: #3