Skip to content
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

feat: connection highlighting in geras and thrasos #7698

Conversation

BeksOmega
Copy link
Collaborator

The basics

The details

Resolves

Work on #7204

Proposed Changes

Adds logic to geras and thrasos to highlight their connections.

Added logic to the base renderer to support code reuse.

Removed the interface since I added the logic to the base renderer.

Reason for Changes

Needed to move highlighting logic to the renderer so it can differ on a renderer-by-renderer-basis.

Test Coverage

Tested that connections in thrasos and geras are still properly highlighted.

Documentation

N/A

Additional Information

N/A

@BeksOmega BeksOmega requested a review from a team as a code owner December 6, 2023 22:57
@BeksOmega BeksOmega requested review from cpcallen and removed request for a team December 6, 2023 22:57
@github-actions github-actions bot added the PR: feature Adds a feature label Dec 6, 2023
@BeksOmega BeksOmega changed the title feat: connection highlighting geras feat: connection highlighting in geras and thrasos Dec 6, 2023
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Dec 6, 2023
Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern is about idempotency (or not) of some of the new methods introduced (see below).

Also not sure why Prettier is suddenly attacking jst2s especially as it appears you did not change anything in that script.

core/rendered_connection.ts Outdated Show resolved Hide resolved
core/renderers/common/constants.ts Show resolved Hide resolved
core/renderers/common/drawer.ts Outdated Show resolved Hide resolved
core/renderers/common/info.ts Outdated Show resolved Hide resolved
core/renderers/common/renderer.ts Outdated Show resolved Hide resolved
core/renderers/common/renderer.ts Show resolved Hide resolved
scripts/migration/js2ts Show resolved Hide resolved
Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for line length issue in jst2ts.

@BeksOmega BeksOmega merged commit fe7a029 into google:connection-previewers Dec 12, 2023
6 checks passed
BeksOmega added a commit to BeksOmega/blockly that referenced this pull request Feb 2, 2024
* chore: move connection highlighting into the geras renderer

* chore: remove IConnectionHighlighter interface

* chore: format

* chore: fixup

* chore: format

* fix: PR comments
BeksOmega added a commit that referenced this pull request Feb 2, 2024
* chore: move connection highlighting into the geras renderer

* chore: remove IConnectionHighlighter interface

* chore: format

* chore: fixup

* chore: format

* fix: PR comments
@BeksOmega BeksOmega deleted the feat/connection-highlighting-geras branch May 14, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants