fix!: removed redundant blockyTreeRowContentContainer div #8373

deysandip301 · 2024-07-16T06:43:55Z

The basics

I validated my changes

The details

Resolves

Proposed Changes

removed the extra attribute rowContents_ from ToolboxCategory class
and added the icons and lables of categories directly to the rowDiv_ attributes

Reason for Changes

the div blockyTreeRowContentContainer was redundant which was used to style but was not given css styling

Test Coverage

Documentation

Additional Information

google-cla · 2024-07-16T06:44:00Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

conventional-commit-lint-gcf · 2024-07-16T06:44:01Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

github-actions

Welcome! It looks like this is your first pull request in Blockly, so here are a couple of tips:

You can find tips about contributing to Blockly and how to validate your changes on our developer site.
All contributors must sign the Google Contributor License Agreement (CLA). If the google-cla bot leaves a comment on this PR, make sure you follow the instructions.
We use conventional commits to make versioning the package easier. Make sure your commit message is in the proper format or learn how to fix it.
If any of the other checks on this PR fail, you can click on them to learn why. It might be that your change caused a test failure, or that you need to double-check the style guide.
Thank you for opening this PR! A member of the Blockly team will review it soon.

BeksOmega · 2024-07-16T17:43:04Z

core/toolbox/category.ts

@@ -195,19 +191,15 @@ export class ToolboxCategory
    aria.setState(this.htmlDiv_, aria.State.LEVEL, this.level_ + 1);

    this.rowDiv_ = this.createRowContainer_();
-    this.rowDiv_.style.pointerEvents = 'auto';
+    this.rowDiv_.style.pointerEvents = 'none';


This makes it so the categories are no longer clickable :/ Could you test this manually using npm run start and see if pointerEvents = 'none'; actually needs to be assigned anywhere or if we can remove it?

Hey @BeksOmega I tested it in local machine using npm run start and it was acting weirdly
and I think the blockyTreeRowContentContainer div is kinda required unless it is breaking
but we can remove the rowContents_ attributes from the ToolboxCategory class it is redundant...

if you can think of any implementation by which we can remove the blockyTreeRowContentContainer div you can comment I will try it out....

Can you try having the rowDiv_.style.pointerEvents = 'auto' while the iconDom_ and the labelDom_ have it set to none? I think that might fix the issue.

Hey @BeksOmega sorry for late reply to this... this week was a little busy ....
I have set the iconDom_ and labelDom_ pointerEvents to none and for the rowDiv_ to auto and it is working fine for me...
please review and suggest for changes if any needed.....

BeksOmega

Looks great! Once this passes CI I'll get this merged =)

BeksOmega · 2024-07-22T23:07:49Z

Looks like this failed formatting. You can fix that by running npm run format in the root of the repository, and committing the fixes.

BeksOmega · 2024-07-23T15:30:36Z

Thank you again for your work on this!

BeksOmega · 2024-07-23T15:32:59Z

Ah actually this is also against the wrong branch :/ It needs to merge into rc/v12.0.0. Could you rebase your changes onto that branch for me? If you've never done a rebase before I can give you the commands.

deysandip301 · 2024-07-23T16:13:03Z

I have not done rebase before
it would be great if you could give me the steps how to do it .....
I am new to contributing and I am very thankful for your help throughout the process.... :)

BeksOmega · 2024-07-23T21:21:25Z

Hello! To rebase onto the rc/v12.0.0 branch, you can follow these steps.

Fetch the latest changes from rc/v12.0.0:
```
git fetch upstream rc/v12.0.0
```
Checkout your branch:
```
git checkout [your-branch-name]
```
Start an interactive rebase:
```
git rebase -i upstream/rc/v12.0.0
```
In the editor that opens, identify the commits that aren't yours. These are likely the ones at the top of the list.
Change the word pick to drop for each commit you want to remove.
Save and close the editor.
Force push your changes to your branch: Note that force pushing is a dangerous operation because it overwrites commit history, so if you dropped the wrong commits you could lose work.
```
git push -f origin [your-branch-name]
```

Once you've done this, the PR should be on the correct branch.

Let me know if you have any questions or run into any issues!

deysandip301 · 2024-07-24T16:34:59Z

I have done the steps I hope it will work correctly...
If you find anything wrong you can comment....
and thank you for all the help during the process....

BeksOmega

Looks like some of the automated tests are failing. Could you look into why and fix them?

BeksOmega · 2024-07-30T02:10:00Z

@deysandip301 Are you still interested in working on this?

deysandip301 · 2024-07-30T05:44:43Z

@BeksOmega Ya, I am working on this, although I am having some trouble while figuring out why some tests are failing. I have fixed the formatting issue, will push it in sometime but why some mocha tests are failing is not something that I am able to get, if you could help me with it then it will be great.

BeksOmega · 2024-07-31T01:24:49Z

Sorry missed this yesterday. Were you able to run the tests locally with npm run test:mocha:interactive? Can you tell me what you tried for debugging from there?

BeksOmega · 2024-08-05T21:35:06Z

@deysandip301 Sorry this has been rough to try to get in! Are you still interested in working on this?

BeksOmega · 2024-08-08T22:10:20Z

I'm gonna go ahead and close this. If you want to continue working on it feel free to reopen!

deysandip301 requested a review from a team as a code owner July 16, 2024 06:43

deysandip301 requested a review from BeksOmega July 16, 2024 06:43

github-actions bot assigned BeksOmega Jul 16, 2024

github-actions bot reviewed Jul 16, 2024

View reviewed changes

deysandip301 changed the title ~~fix : removed redundant blockyTreeRowContentConatiner div~~ fix: removed redundant blockyTreeRowContentConatiner div Jul 16, 2024

github-actions bot added the PR: fix Fixes a bug label Jul 16, 2024

BeksOmega requested changes Jul 16, 2024

View reviewed changes

BeksOmega approved these changes Jul 22, 2024

View reviewed changes

BeksOmega changed the title ~~fix: removed redundant blockyTreeRowContentConatiner div~~ fix!: removed redundant blockyTreeRowContentConatiner div Jul 23, 2024

github-actions bot added breaking change Used to mark a PR or issue that changes our public APIs. PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 23, 2024

BeksOmega changed the title ~~fix!: removed redundant blockyTreeRowContentConatiner div~~ fix!: removed redundant blockyTreeRowContentContainer div Jul 23, 2024

github-actions bot added breaking change Used to mark a PR or issue that changes our public APIs. PR: fix Fixes a bug and removed PR: fix Fixes a bug breaking change Used to mark a PR or issue that changes our public APIs. labels Jul 23, 2024

deysandip301 added 4 commits July 24, 2024 21:55

fix:removed redundant rowContents_ div

a2f236c

removed redundant rowContents_ div

2d97382

fix:resolved pointerEvents issue

e62594d

resolved formatting issue

6cf159d

deysandip301 force-pushed the issue8346 branch from ccf8f63 to 6cf159d Compare July 24, 2024 16:33

BeksOmega changed the base branch from develop to rc/v12.0.0 July 25, 2024 01:06

github-actions bot added breaking change Used to mark a PR or issue that changes our public APIs. PR: fix Fixes a bug and removed PR: fix Fixes a bug breaking change Used to mark a PR or issue that changes our public APIs. labels Jul 25, 2024

BeksOmega requested changes Jul 25, 2024

View reviewed changes

fix: fixed formatting issues

9639674

BeksOmega closed this Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: removed redundant blockyTreeRowContentContainer div #8373

fix!: removed redundant blockyTreeRowContentContainer div #8373

deysandip301 commented Jul 16, 2024 •

edited

Loading

google-cla bot commented Jul 16, 2024

conventional-commit-lint-gcf bot commented Jul 16, 2024

github-actions bot left a comment

BeksOmega Jul 16, 2024

deysandip301 Jul 16, 2024

BeksOmega Jul 17, 2024

deysandip301 Jul 22, 2024

BeksOmega left a comment

BeksOmega commented Jul 22, 2024

BeksOmega commented Jul 23, 2024

BeksOmega commented Jul 23, 2024

deysandip301 commented Jul 23, 2024

BeksOmega commented Jul 23, 2024

deysandip301 commented Jul 24, 2024

BeksOmega left a comment

BeksOmega commented Jul 30, 2024

deysandip301 commented Jul 30, 2024

BeksOmega commented Jul 31, 2024

BeksOmega commented Aug 5, 2024

BeksOmega commented Aug 8, 2024

fix!: removed redundant blockyTreeRowContentContainer div #8373

fix!: removed redundant blockyTreeRowContentContainer div #8373

Conversation

deysandip301 commented Jul 16, 2024 • edited Loading

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

google-cla bot commented Jul 16, 2024

conventional-commit-lint-gcf bot commented Jul 16, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

BeksOmega Jul 16, 2024

Choose a reason for hiding this comment

deysandip301 Jul 16, 2024

Choose a reason for hiding this comment

BeksOmega Jul 17, 2024

Choose a reason for hiding this comment

deysandip301 Jul 22, 2024

Choose a reason for hiding this comment

BeksOmega left a comment

Choose a reason for hiding this comment

BeksOmega commented Jul 22, 2024

BeksOmega commented Jul 23, 2024

BeksOmega commented Jul 23, 2024

deysandip301 commented Jul 23, 2024

BeksOmega commented Jul 23, 2024

deysandip301 commented Jul 24, 2024

BeksOmega left a comment

Choose a reason for hiding this comment

BeksOmega commented Jul 30, 2024

deysandip301 commented Jul 30, 2024

BeksOmega commented Jul 31, 2024

BeksOmega commented Aug 5, 2024

BeksOmega commented Aug 8, 2024

deysandip301 commented Jul 16, 2024 •

edited

Loading