allow setting label font #174
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
This was referenced Dec 16, 2024
Merged
This stack of pull requests is managed by Graphite. Learn more about stacking. |
bmschmidt
force-pushed
the
11-11-refactor_manifests_to_be_lazier
branch
from
0491be5 to
cb409c1
Compare
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
81981a0 to
1bbf81a
Compare
This was referenced Dec 18, 2024
Draft
Draft
bmschmidt
force-pushed
the
11-11-refactor_manifests_to_be_lazier
branch
from
cb409c1 to
cdf64ac
Compare
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
1bbf81a to
de1e52d
Compare
bmschmidt
force-pushed
the
11-11-refactor_manifests_to_be_lazier
branch
from
cdf64ac to
ddc25b7
Compare
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
de1e52d to
be9abee
Compare
bmschmidt
force-pushed
the
11-11-refactor_manifests_to_be_lazier
branch
from
ddc25b7 to
f7580eb
Compare
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
be9abee to
c6fdf4b
Compare
bmschmidt
force-pushed
the
11-11-refactor_manifests_to_be_lazier
branch
from
f7580eb to
903c8a9
Compare
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
c6fdf4b to
c97bb6a
Compare
bmschmidt
force-pushed
the
11-11-refactor_manifests_to_be_lazier
branch
from
903c8a9 to
eec66a0
Compare
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
c97bb6a to
26e697c
Compare
RLesser
approved these changes
May 24, 2025
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
26e697c to
a469515
Compare
This was referenced May 26, 2025
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed a469515 in 1 minute and 52 seconds. Click for details.
- Reviewed
114lines of code in3files - Skipped
0files when reviewing. - Skipped posting
11draft 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. src/interaction.ts:172
- Draft comment:
Minor formatting: Removed semicolon after the zoom event handler callback. Ensure consistency with coding style. - Reason this comment was not posted:
Confidence changes required:33%<= threshold70%None
2. src/label_rendering.ts:32
- Draft comment:
Added a new private 'font' property with default 'verdana'. This improves configurability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
3. src/label_rendering.ts:47
- Draft comment:
Set the label font from options if provided, otherwise default to 'verdana'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
4. src/label_rendering.ts:80
- Draft comment:
Passing 'this.font' to the DepthTree constructor ensures consistent font usage for label measurements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
5. src/label_rendering.ts:271
- Draft comment:
Updated canvas text rendering to use the dynamic font property instead of hardcoding 'verdana'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
6. src/label_rendering.ts:387
- Draft comment:
Modified 'measure_text' to accept a font parameter and use it when setting context.font. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
7. src/label_rendering.ts:434
- Draft comment:
Added a 'font' member to DepthTree to propagate the label font setting. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
8. src/label_rendering.ts:448
- Draft comment:
DepthTree constructor now accepts a 'font' parameter and assigns it to the instance. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
9. src/label_rendering.ts:530
- Draft comment:
Updated the insert_point method to pass the font to measure_text, ensuring proper measurement. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
10. src/types.ts:516
- Draft comment:
Extended LabelOptions with an optional 'font' property (default 'verdana') for label configuration. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
11. src/interaction.ts:175
- Draft comment:
Typographical issue: A semicolon seems to be missing after the closing parenthesis on this line. This is inconsistent with similar lines in the file (e.g., line 177). Please consider adding a semicolon for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 70% While the comment points out an actual inconsistency in semicolon usage, this is a very minor stylistic issue that would typically be handled by a linter or code formatter. The code will work fine either way. None of the priority rules mention semicolon consistency. This seems like the kind of nitpicky comment we should avoid. The code base may have specific style guidelines around semicolon usage that I'm not aware of. Inconsistent style could make the code harder to maintain. Even if there are style guidelines, this is exactly the kind of thing that should be enforced by automated tools like ESLint or Prettier, not manual code review comments. This comment about semicolon consistency is too minor and should be handled by automated tools rather than code review.
Workflow ID: wflow_JzJcbSmYQMjunSv5
You can customize 
bmschmidt
changed the base branch from
11-11-refactor_manifests_to_be_lazier
to
graphite-base/174
bmschmidt
force-pushed
the
graphite-base/174
branch
from
298fe3a to
530be52
Compare
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
a469515 to
a10486b
Compare
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
a10486b to
5d28dde
Compare
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
838f700 to
3d173f5
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3d173f5 in 50 seconds. Click for details.
- Reviewed
34lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft 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. src/types.ts:8
- Draft comment:
Removed unused 'TypeMap' from the Apache Arrow import. Confirm it's not needed elsewhere. - 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. src/types.ts:520
- Draft comment:
Added missing semicolon in LabelOptions for 'font'; ensures consistent syntax. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold70%This comment is purely informative, as it only states that a semicolon was added for consistency. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue.
3. src/wrap_arrow.ts:17
- Draft comment:
Removed duplicate 'extent' import from d3-array; only one instance is required. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold70%This comment is purely informative, stating that a duplicate import was removed. It doesn't provide a suggestion or ask for confirmation about an intentional change. It violates the rule against making purely informative comments.
Workflow ID: wflow_mP8mLKdnzksdaca2
You can customize
bmschmidt
force-pushed
the
12-16-allow_setting_label_font
branch
from
3d173f5 to
bdea4f6
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed bdea4f6 in 47 seconds. Click for details.
- Reviewed
47lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft 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. package.json:4
- Draft comment:
Version downgrade detected. Downgrading from '3.0.0-next.50' to '3.0.0-next.47' can confuse dependency resolution. Bump the version instead. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold70%This comment is related to dependency version changes, which is not allowed according to the rules. The comment suggests a version bump instead of a downgrade, but since it involves dependency management, it should be ignored.
2. src/types.ts:522
- Draft comment:
Minor formatting: added semicolon after 'font?: string'. This improves consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
3. src/wrap_arrow.ts:17
- Draft comment:
Removed duplicate import of 'extent' from 'd3-array'. This cleanup is good. - Reason this comment was not posted:
Confidence changes required:0%<= threshold70%None
4. src/types.ts:522
- Draft comment:
Ensure the new 'font' option in LabelOptions is utilized in label rendering. Currently, it's only defined in types. - Reason this comment was not posted:
Confidence changes required:66%<= threshold70%None
Workflow ID: wflow_x87kdFAtN7dd0OyA
You can customize
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "deepscatter", | |||
| "type": "module", | |||
| "version": "3.0.0-next.50", | |||
| "version": "3.0.0-next.47", | |||
There was a problem hiding this comment.
The package version in this PR is being downgraded from 3.0.0-next.50 to 3.0.0-next.47. This appears to be unintentional, as adding new features (like custom label fonts) should typically be accompanied by a version increment rather than a decrement. Please ensure the version number is properly incremented according to the project's versioning policy.
Suggested change
| "version": "3.0.0-next.47", | |
| + "version": "3.0.0-next.51", |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Merge activity
|
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.
Important
Add support for setting label font in Deepscatter via
fontoption inLabelMaker.fontoption inLabelMakerinlabel_rendering.ts.measure_text()inlabel_rendering.tsto use specifiedfont.DepthTreeconstructor inlabel_rendering.tsto acceptfontparameter.fontoption toLabelOptionsintypes.ts.wrap_arrow.ts.This description was created by
for bdea4f6. You can customize this summary. It will automatically update as commits are pushed.