-
Notifications
You must be signed in to change notification settings - Fork 1k
use DomElementStyles #1810
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
Merged
Merged
use DomElementStyles #1810
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Important
Looks good to me! 👍
Reviewed everything up to 580ae4e in 1 minute and 56 seconds. Click for details.
- Reviewed
212
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
15
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/studio/electron/preload/webview/elements/style.ts:1
- Draft comment:
Good update: using DomElementStyles as return type. Ensure that default inline/stylesheet merging (lines 10-15) produces correct styles with defaults (like auto width/height). - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. apps/studio/src/lib/editor/engine/element/index.ts:85
- Draft comment:
Passing domEl.styles directly instead of merging computed/defined is acceptable now. Confirm that downstream consumers (e.g., ClickRect) handle a null styles value gracefully. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. apps/studio/src/lib/editor/engine/overlay/index.ts:30
- Draft comment:
The clickRects array now uses styles as 'DomElementStyles | null'. Ensure that all consumers use optional chaining (e.g., styles?.computed) to avoid runtime errors. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. apps/studio/src/lib/editor/engine/overlay/state.ts:11
- Draft comment:
Updated ClickRectState now explicitly types styles as DomElementStyles | null. This is consistent; just ensure downstream components check for null. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. apps/studio/src/routes/editor/Canvas/Overlay/ClickRect.tsx:305
- Draft comment:
Access to defined styles for dimension labels is now done via styles?.defined.width. Verify that the intended fallback behavior occurs when styles is null. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. apps/studio/src/routes/editor/Canvas/Overlay/ClickRect.tsx:359
- Draft comment:
Using styles?.computed['borderRadius'] in ResizeHandles is correct, but confirm that undefined values are handled as intended. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. apps/web/preload/script/api/elements/style.ts:1
- Draft comment:
Consistent type usage with DomElementStyles in the web preload script. No issues noted. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. packages/models/src/element/element.ts:13
- Draft comment:
DomElement interface now uses DomElementStyles. This unifies style handling. Confirm that backward compatibility is maintained where applicable. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
9. General:0
- Draft comment:
PR description mentions adding better onboarding docs, but the changes focus on using DomElementStyles. Make sure the documentation updates are intended, or update the PR description accordingly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. apps/studio/electron/preload/webview/elements/style.ts:5
- Draft comment:
getStyles now returns a DomElementStyles object. Ensure that merging inline and stylesheet styles (lines 10–15) properly handles conflicts and that all expected style properties are captured. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
11. apps/studio/src/lib/editor/engine/element/index.ts:88
- Draft comment:
Click handling now passesdomEl.styles
directly. Verify that downstream consumers properly handle a null value for styles. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
12. apps/studio/src/lib/editor/engine/overlay/state.ts:11
- Draft comment:
ClickRectState now requires a 'styles' field of type DomElementStyles | null. Ensure all consumers check for a null value before accessing sub-properties. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
13. apps/studio/src/routes/editor/Canvas/Overlay/ClickRect.tsx:305
- Draft comment:
Usage of optional chaining (e.g.styles?.defined.width
) ensures safe fallback when styles is null. Confirm that both 'defined' and 'computed' properties are populated as expected when styles are present. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
14. apps/studio/src/routes/editor/Canvas/Overlay/index.tsx:64
- Draft comment:
PassingrectState.styles
without a fallback is now acceptable as consumers are updated; ensure that components handle a null styles value gracefully. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
15. GLOBAL:1
- Draft comment:
The PR description references onboarding docs improvements, but the code changes focus on using DomElementStyles. Please update the PR description to accurately reflect these changes. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ApMDodIzTQ7mh6JP
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
zongkelong
pushed a commit
to zongkelong/coolook
that referenced
this pull request
Apr 25, 2025
* 'main' of https://github.com/onlook-dev/onlook: use DomElementStyles (onlook-dev#1810) feat: Added file watcher & auto update modified opened files in Dev Tab (onlook-dev#1802) fix: border thickness hidden when setting border color from color picker (onlook-dev#1805) move overflow input (onlook-dev#1800) Added overflow option in the style panel (onlook-dev#1793) refactor: update images manager (onlook-dev#1784) chore: error nesting button (onlook-dev#1790) refactor: update naming for brand color (onlook-dev#1785) Tys toolbar v4 (onlook-dev#1786)
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Refactor style handling by introducing
DomElementStyles
type and updating related code to use this new type.DomElementStyles
type inelement.ts
to encapsulatedefined
andcomputed
styles.getStyles()
instyle.ts
(bothapps/studio/electron/preload/webview/elements
andapps/web/preload/script/api/elements
) to returnDomElementStyles
.ElementManager
inindex.ts
to useDomElementStyles
for style handling.OverlayManager
andOverlayState
inoverlay/index.ts
andoverlay/state.ts
to handleDomElementStyles
.ClickRect
component inClickRect.tsx
to utilizeDomElementStyles
for rendering styles.computed
anddefined
styles inElementManager
andOverlayManager
.This description was created by
for 580ae4e. You can customize this summary. It will automatically update as commits are pushed.