feat: apply font to elements #1719
Merged
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to c3aa555 in 3 minutes and 44 seconds
More details
- Looked at
635lines of code in12files - Skipped
1files when reviewing. - Skipped posting
16drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/font.ts:89
- Draft comment:
Good use of kebabCase for generating the CSS variable. Confirm that fallback and preload values are correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/src/routes/editor/EditPanel/StylesTab/single/FontInput.tsx:15
- Draft comment:
Ensure that convertFontString handles unexpected font strings correctly (e.g., if formatting differs from expected). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/studio/src/routes/editor/LayersPanel/index.tsx:56
- Draft comment:
There is a TODO comment regarding mouse leave behavior; ensure to implement proper handling for consistent UX. - 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.
4. apps/studio/src/lib/editor/engine/index.ts:41
- Draft comment:
New getter and setter for layersPanelTab look correct; verify that updating this property triggers necessary UI updates. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. apps/studio/src/lib/editor/styles/group.ts:244
- Draft comment:
The font style group now uses a SingleStyleImpl for fontFamily. Verify that it integrates well with your new FontInput component. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. apps/studio/electron/main/assets/fonts/font.ts:25
- Draft comment:
Using replace to generate importName may lead to collisions. Consider using camelCase or kebabCase so that the identifier is more consistent. - 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.
7. apps/studio/electron/main/assets/fonts/font.ts:288
- Draft comment:
Using fs.unlinkSync in an async function can block the event loop. Consider using an asynchronous alternative (e.g., fs.promises.unlink). - 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.
8. apps/studio/src/lib/editor/engine/index.ts:60
- Draft comment:
Several managers are instantiated during construction. Consider lazy instantiation if performance is a concern. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
9. apps/studio/src/lib/editor/engine/style/index.ts:136
- Draft comment:
Usage of Array.prototype.toSorted() is modern. Ensure the target Electron version fully supports it or consider polyfilling. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
10. apps/studio/src/routes/editor/EditPanel/StylesTab/index.tsx:200
- Draft comment:
The inline defaultValue ([...Object.values(StyleGroupKey), 'tw']) is not self‐descriptive. Consider defining a named constant for clarity. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
11. apps/studio/src/routes/editor/LayersPanel/index.tsx:56
- Draft comment:
There is a TODO in handleMouseLeave regarding dropdown behavior. Ensure this is addressed to reliably detect mouse leave from the content panel. - 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.
12. apps/studio/common/helpers/twTranslator/index.ts:475
- Draft comment:
Typo: In the 'hue-rotate' function for the 'backdrop-filter' property (around line 475), the code usescustomTheme['backdrop-grayscale']instead ofcustomTheme['backdrop-hue-rotate']. This appears to be a mistake and should be corrected. - 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.
13. apps/studio/common/helpers/twTranslator/index.ts:1113
- Draft comment:
Typo: In the 'hue-rotate' function for the 'filter' property (around line 1113), the code incorrectly accessescustomTheme['grayscale']instead ofcustomTheme['hue-rotate']. This is likely an unintentional mistake and should be fixed. - 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.
14. apps/studio/src/lib/editor/styles/group.ts:245
- Draft comment:
There's an extra blank line following the new 'fontFamily' style in the TextGroup array. While this is a minor formatting issue, it could be removed to maintain consistency with the rest of the file. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is technically correct - there is an extra blank line. However, this is an extremely minor formatting issue. Looking at our rules, we should not make comments that are obvious or unimportant. Code formatting issues like this are typically handled by automated formatters or linters.
The comment is factually accurate. Perhaps consistent spacing is important for code readability and maintainability?
While consistent spacing is important, this is too minor of an issue to warrant a PR comment. This kind of formatting should be handled by automated tools rather than manual review comments.
Delete this comment as it points out an extremely minor formatting issue that doesn't meaningfully impact code quality.
15. apps/studio/src/routes/editor/LayersPanel/index.tsx:12
- Draft comment:
Minor naming inconsistency: 'ImagesTab' is imported from './ImageTab'. Consider verifying if the file name should be plural (e.g. './ImagesTab') for consistency with the usage and other similar imports. - 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.
16. apps/studio/src/routes/editor/LayersPanel/index.tsx:15
- Draft comment:
Minor naming inconsistency: 'PagesTab' is imported from './PageTab'. Please confirm if the file should be named './PagesTab' to match the plural usage in translations and other component names. - 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_rY32AZYvpug7kCRZ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -3416,6 +3423,7 @@ const getResultCode = (it: CssCodeParse, prefix = '', config: TranslatorConfig) | |||
| .join(' '); | |||
| } | |||
| } | |||
| console.log('pipeVal', pipeVal); | |||
There was a problem hiding this comment.
Remove or disable the debugging console.log before merging to production.
Suggested change
| console.log('pipeVal', pipeVal); |
spartan-vutrannguyen
force-pushed
the
feat/apply-font-to-elements
branch
from
8200e83 to
67fbf67
Compare
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 73d6933 in 2 minutes and 8 seconds
More details
- Looked at
4033lines of code in16files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. packages/utility/src/font.ts:187
- Draft comment:
Good use of regex and lodash.camelCase in convertFontString. The function clearly strips the '__' prefix and hex hash, then applies camelCase. Consider adding a brief inline note on expected inputs. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. packages/utility/src/font.ts:193
- Draft comment:
Using replace(/_[a-f0-9]+$/, '') efficiently strips trailing hash. Ensure that input fonts always follow this pattern. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. packages/utility/src/font.ts:187
- Draft comment:
The new convertFontString function is clear and concise. Consider adding unit tests to cover edge cases (e.g. inputs that don't match the expected pattern) to ensure robustness. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:60
- Draft comment:
There's an extra space between 'cursor-pointer' and 'max-w-52' in the className attribute on line 60. Consider removing the additional space to keep the styling classes consistent. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
Extra spaces in className strings don't affect the rendering or functionality. While it's a minor style inconsistency, it doesn't impact code quality or behavior. This feels like an extremely trivial comment that doesn't warrant developer attention. The actual className changes (adding flex-1 and max-w-52) are more significant but not problematic.
Perhaps consistent code formatting is important for maintainability and readability? Maybe the team has strict style guidelines about className formatting?
Even if style consistency matters, this is too minor - most teams use Prettier or similar tools to handle formatting, and extra spaces in className strings are not a meaningful issue worth a PR comment.
Delete this comment as it points out an extremely trivial formatting issue that doesn't impact functionality or meaningful code quality.
5. packages/models/src/constants/fonts.ts:17680
- Draft comment:
The 'rocknroll-one' entry has a defSubset value of 'japanese' but the subsets array only includes ['cyrillic', 'latin', 'latin-ext']. Please verify whether 'japanese' should be added to the subsets array or if the defSubset should be updated to one of the already listed subsets. - 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.
6. packages/utility/src/font.ts:49
- Draft comment:
The word 'kursiv' in the styleTerms array might be a typographical error or an intentional non-English variant. If it's intentional for supporting specific font naming conventions (e.g., for German), please add a clarifying comment. Otherwise, consider correcting it to a more standard term (like 'italic' or 'cursive'). - 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_lEpJxmiuyNTcwAw7
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| t.objectProperty(t.identifier('display'), t.stringLiteral('swap')), | ||
| t.objectProperty( | ||
| t.identifier('fallback'), | ||
| t.arrayExpression([t.stringLiteral('system-ui'), t.stringLiteral('san-serif')]), |
There was a problem hiding this comment.
Typo detected in the fallback font configuration: 'san-serif' should likely be corrected to 'sans-serif' to match the standard naming.
Suggested change
| t.arrayExpression([t.stringLiteral('system-ui'), t.stringLiteral('san-serif')]), | |
| t.arrayExpression([t.stringLiteral('system-ui'), t.stringLiteral('sans-serif')]), |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8d5c5f7 in 1 minute and 20 seconds
More details
- Looked at
28lines of code in2files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/font.ts:290
- Draft comment:
Consider using asynchronous fs methods (e.g. from fs/promises) instead of synchronous calls (fs.writeFileSync, fs.unlinkSync) in these async functions to avoid blocking the event loop. - 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.
2. apps/studio/electron/main/assets/fonts/font.ts:317
- Draft comment:
Multiple AST traversals (e.g. checking/updating imports and exports) occur in addFont and addLocalFont. Consider consolidating them to improve readability and performance. - 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.
3. apps/studio/electron/main/assets/fonts/font.ts:277
- Draft comment:
The removal of the localFont import uses a regex on the generated code. This may be prone to unintended matches. Consider handling removal via AST manipulation for robustness. - 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.
4. apps/studio/electron/main/assets/fonts/font.ts:25
- Draft comment:
Consider using a consistent naming convention (e.g. camelCase) for the import function name instead of simply replacing spaces with underscores. - 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.
5. apps/studio/electron/main/assets/fonts/font.ts:354
- Draft comment:
Guard against baseFontName being undefined. If extractFontParts returns undefined, fontName becomes 'custom-undefined'. - 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.
6. apps/studio/electron/main/assets/fonts/font.ts:387
- Draft comment:
Consider using asynchronous file write instead of fs.writeFileSync to remain consistent with the async pattern. - 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.
7. apps/studio/electron/main/assets/fonts/font.ts:289
- Draft comment:
Using synchronous fs.unlinkSync to delete font files may block the event loop; consider using an async alternative. - 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.
8. apps/studio/electron/main/assets/fonts/font.ts:416
- Draft comment:
The Babel AST manipulation code is quite detailed; consider extracting helper functions for repeated tasks (e.g., updating import declarations) to improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
Workflow ID: wflow_KL2uyF0JDBJHdTaa
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on db9fdea in 1 minute and 10 seconds
More details
- Looked at
31lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/studio/src/routes/editor/WebviewArea/Frame.tsx:97
- Draft comment:
Removed commented-out code cleanup. Ensure this commented block isn’t needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/src/routes/editor/WebviewArea/Frame.tsx:97
- Draft comment:
PR description suggests adding a Font Input feature, but no related code is visible in this diff. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to update the PR description or to explain the discrepancy between the PR description and the code changes. This violates the rule against asking the PR author to update the PR description or to confirm their intention.
3. apps/studio/src/routes/editor/WebviewArea/Frame.tsx:97
- Draft comment:
Removed commented-out code (selectFirstElement call). This cleanup is fine if that logic isn’t needed. Verify that no required behavior was inadvertently removed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify that no required behavior was removed after deleting some commented-out code. This falls under the rule of not asking the author to double-check things or ensure behavior is intended. The comment does not provide a specific suggestion or point out a specific issue.
4. apps/studio/src/routes/editor/WebviewArea/Frame.tsx:384
- Draft comment:
Removed the unused async function 'selectFirstElement'. This cleanup reduces legacy code—confirm this functionality isn’t needed for upcoming features. - 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.
5. apps/studio/src/routes/editor/WebviewArea/Frame.tsx:1
- Draft comment:
The PR title and description mention adding a font input and updating onboarding docs, but this diff only shows cleanup removals. Please ensure that all intended feature and documentation changes are included. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_GePG4nXMQY5G70Kf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
* refactor: display font as their style * using debounce for search * add font convert test * update variable naming
Kitenite
reviewed
Apr 4, 2025
| @@ -48,25 +37,63 @@ const FontPanel = observer(({ onClose }: FontPanelProps) => { | |||
| fontManager.uploadFonts(fonts); | |||
| }; | |||
|
|
|||
| const performSearch = useCallback( | |||
There was a problem hiding this comment.
The first search is really slow. Not blocking but fyi
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* apply color to element * update font files * open font panel when click add new font * update font file * fix write font to prevent data lost * reload webview when apply font * format font when undo and redo action * update fonts file * remove logs * refactor: display font as their style (onlook-dev#1729) * refactor: display font as their style * using debounce for search * add font convert test * update variable naming
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* apply color to element * update font files * open font panel when click add new font * update font file * fix write font to prevent data lost * reload webview when apply font * format font when undo and redo action * update fonts file * remove logs * refactor: display font as their style (onlook-dev#1729) * refactor: display font as their style * using debounce for search * add font convert test * update variable naming
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)
Screen.Recording.2025-04-04.at.23.11.03.mp4
Additional Notes
Important
Add font application feature to editor with
FontInputcomponent and manage font-family values and configurations.FontInputcomponent inStylesTab/index.tsxto apply fonts to selected elements.font-familyvalues intwTranslator/index.tswithout square brackets.updateFontFamily()instyle/index.tsto update font styles.LayersPanelTabValueenum inmodels.tsfor managing layers panel tabs.convertFontString()infont.tsto convert font strings to camel case.fontFamiliesinfonts.tsto remove unnecessary subsets and entries.This description was created by
for db9fdea. It will automatically update as commits are pushed.