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: added blocklyHighlighted CSS class to highlighted block's root… #8407

Merged
merged 5 commits into from
Aug 14, 2024

Conversation

Shreshthaaa
Copy link

Details :

Added blocklyHighlighted CSS class to highlighted block's root svg.

Fixes:

#8263

@Shreshthaaa Shreshthaaa requested a review from a team as a code owner July 21, 2024 09:00
@Shreshthaaa Shreshthaaa requested a review from gonfunko July 21, 2024 09:00
@github-actions github-actions bot added the PR: fix Fixes a bug label Jul 21, 2024
@Shreshthaaa Shreshthaaa changed the title fix: added 'blocklyHighlighted' CSS class to highlighted block's root… fix: added blocklyHighlighted CSS class to highlighted block's root… Jul 21, 2024
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 21, 2024
@gonfunko gonfunko requested review from BeksOmega and removed request for gonfunko July 29, 2024 22:51
@gonfunko gonfunko removed their assignment Jul 29, 2024
Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Thanks for your work! Just one fix.

Comment on lines 172 to 178
addClass(element: SVGElement, className:string) {
element.classList.add(className);
}

removeClass(element: SVGElement, className: string) {
element.classList.remove(className);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you remove these and call the existing setClass method instead?

@BeksOmega
Copy link
Collaborator

Heya @Shreshthaaa Are you still interested in working on this? (and your other open PRs?)

@Shreshthaaa
Copy link
Author

Heya @Shreshthaaa Are you still interested in working on this? (and your other open PRs?)

Hi @BeksOmega , thank you for following up. I apologize for the delay. I've been quite busy lately but I am still interested in working on this and my other open PRs. I will address the requested changes in a day or two.

@Shreshthaaa
Copy link
Author

@BeksOmega Hey, I have done the said changes. Kindly review.

Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Just pulled this down locally to test and I don't think this is working for the renders/geras/path_object. Can you refactor that to properly call super?

@Shreshthaaa
Copy link
Author

Just pulled this down locally to test and I don't think this is working for the renders/geras/path_object. Can you refactor that to properly call super?

Hey, I made changes in renderers/geras/path_object itself. Kindly review.

Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Thanks for the changes but that doesn't quite do what I asked! In the geras path object it should call super to add the new CSS class. The only new code in the gears path object should be setting the svgPathLight.style.display values.

@Shreshthaaa
Copy link
Author

Thanks for the changes but that doesn't quite do what I asked! In the geras path object it should call super to add the new CSS class. The only new code in the gears path object should be setting the svgPathLight.style.display values.

Hey, I have called super in the geras path object file to call the parent class method. Kindly check if the code looks correct now.

@@ -103,6 +103,7 @@ export class PathObject extends BasePathObject {
}

override updateHighlighted(highlighted: boolean) {
super.updateHighlighted(highlighted);
if (highlighted) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still needs a change! Like I said the only lines should be the svgPathLight.style.display (and the associated if-else).

You can remove the lines in the geras path object that set the filter, since that already happens in the super function.

@BeksOmega
Copy link
Collaborator

Heya @Shreshthaaa are you still interested in working on this?

@Shreshthaaa
Copy link
Author

Heya @Shreshthaaa are you still interested in working on this?

Hey @BeksOmega, please review the update. Hopefully, everything is in line now. Thank you!

Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you for the changes =) Once this passes CI I'll get it merged.

@BeksOmega BeksOmega changed the title fix: added blocklyHighlighted CSS class to highlighted block's root… feat: added blocklyHighlighted CSS class to highlighted block's root… Aug 14, 2024
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: fix Fixes a bug labels Aug 14, 2024
@BeksOmega BeksOmega merged commit 64fd9ad into google:rc/v12.0.0 Aug 14, 2024
8 checks passed
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.

3 participants