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.

Sign up for GitHub

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

Jump to bottom

Remove the guidata option #413

Merged
merged 22 commits into from
Sep 16, 2024
Merged

Remove the guidata option #413

merged 22 commits into from
Sep 16, 2024

Conversation

arjxn-py
Copy link
Member

Should potentially close #410

@arjxn-py arjxn-py marked this pull request as draft September 12, 2024 10:44
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 13, 2024 •
edited
Loading

Preview PR at appsharing.space

@arjxn-py
Copy link
Member Author

Bot please update snapshots!

@arjxn-py arjxn-py marked this pull request as ready for review September 13, 2024 10:12
@arjxn-py arjxn-py closed this Sep 13, 2024
@arjxn-py arjxn-py reopened this Sep 13, 2024
@arjxn-py arjxn-py changed the title [WIP]: Remove the guidata option Remove the guidata option Sep 13, 2024
martinRenou
martinRenou requested changes Sep 13, 2024
View reviewed changes
Copy link
Member

@martinRenou martinRenou 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 starting this!

I've not looked too deeply into the implementation yet, but it looks like this PR is breaking the some logic upon the application of a boolean operator.
When creating two initial shapes, then applying a cut operator on them, the two initial shapes are not set hidden anymore.
Also when trying to hide them manually from the left panel seems to not work reliably.

I wonder if this is the cause of the visual regression the CI has seen.

@arjxn-py
Copy link
Member Author

I've not looked too deeply into the implementation yet, but it looks like this PR is breaking the some logic upon the application of a boolean operator.
When creating two initial shapes, then applying a cut operator on them, the two initial shapes are not set hidden anymore.
Also when trying to hide them manually from the left panel seems to not work reliably.

I noticed this after your comment, i'm looking into why this is happening. Thanks for pointing it out.

@trungleduc
Copy link
Member

color-error
This error here is related?

@arjxn-py
Copy link
Member Author

This error here is related?

Might be related to #397, have to look into it. Thanks a lot for letting me know

@arjxn-py
Copy link
Member Author

Bot please update snapshots!

@arjxn-py arjxn-py closed this Sep 14, 2024
@arjxn-py arjxn-py reopened this Sep 14, 2024
@arjxn-py
Copy link
Member Author

This error here is related?

Might be related to #397, have to look into it. Thanks a lot for letting me know

Hi @trungleduc, I tried reproducing this on my end locally but it seems like it works fine for me. However I'd want to confirm a couple of things -

  • Did you select any object before clicking intersection? (As it takes the color of that object)
  • Do you also get commands.js:262 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'value') error when you try to do intersection with no obect or a single object? (This should be a valid error but maybe we could handle it better.)

@arjxn-py
Copy link
Member Author

jcad-guidata.mp4
  • Visibility toggling works fine
  • Only result is visible after operation

@trungleduc
Copy link
Member

yeah, I can not reproduce it anymore, thanks for double-checking it!

trungleduc
trungleduc requested changes Sep 14, 2024
View reviewed changes
packages/base/src/commands.ts Outdated Show resolved Hide resolved
packages/base/src/panelview/objecttree.tsx Outdated Show resolved Hide resolved
python/jupytercad_lab/jupytercad_lab/notebook/cad_document.py Outdated Show resolved Hide resolved
@trungleduc
Copy link
Member

we should update https://github.com/jupytercad/JupyterCAD-FreeCAD since it also uses the guidata section

@arjxn-py
Copy link
Member Author

we should update https://github.com/jupytercad/JupyterCAD-FreeCAD since it also uses the guidata section

Yes, I'll be on that next. Thank You :)

martinRenou
martinRenou approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks! Let's make an alpha release of this so we can start updating jupytercad-freecad

@martinRenou martinRenou merged commit 482f0cf into jupytercad:main Sep 16, 2024
9 checks passed
@arjxn-py
Copy link
Member Author

Thanks! Let's make an alpha release of this so we can start updating jupytercad-freecad

Sounds good, should I wait for the release to open a PR on jupytercad-freecad?

@martinRenou
Copy link
Member

That will be easier for you that way 👍🏽

@martinRenou
Copy link
Member

It's out https://github.com/jupytercad/JupyterCAD/releases/tag/v3.0.0a0 You can install it with pip: https://pypi.org/project/jupytercad/3.0.0a0/

@arjxn-py
Copy link
Member Author

It's out https://github.com/jupytercad/JupyterCAD/releases/tag/v3.0.0a0 You can install it with pip: https://pypi.org/project/jupytercad/3.0.0a0/

Best! Thanks @martinRenou 🚀

@arjxn-py arjxn-py deleted the remove-guidata branch September 16, 2024 08:12
This was referenced Sep 16, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trungleduc trungleduc trungleduc approved these changes

@martinRenou martinRenou martinRenou approved these changes

Assignees
No one assigned
Labels
breaking change maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the guidata option
3 participants
@arjxn-py @trungleduc @martinRenou