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

CELE-116 Frontend changes for the EM viewer #76

Merged
merged 15 commits into from
Dec 18, 2024
Merged

Conversation

dvcorreia
Copy link
Member

@dvcorreia dvcorreia commented Dec 6, 2024

screen-recording.mp4

@dvcorreia dvcorreia self-assigned this Dec 6, 2024
const { gray50, gray600, gray400B } = vars;
const CustomFormControlLabel = ({ label, tooltipTitle, helpText }) => {
const CustomFormControlLabel = ({ label, tooltipTitle, helpText, checked, onChange }: CustomFormControlLabel) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we move this somewhere else? It's imported by the EM viewer.
I can also just copy it to the EM viewer.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, I would move it somewhere else instead, if it's shared, it's weird to have it in the 3D viewer only.

@dvcorreia dvcorreia marked this pull request as ready for review December 10, 2024 11:52
@dvcorreia dvcorreia requested a review from aranega December 10, 2024 11:53
@dvcorreia dvcorreia changed the base branch from feature/CELE-117 to develop December 10, 2024 11:53
Copy link
Member

@aranega aranega left a comment

Choose a reason for hiding this comment

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

@dvcorreia Thanks for the first version 🙏
I think it's important though to use the synchronizers and not to rely on special attributes for the features. The fact that currently there is no "behavior" to synapse selection in the 3D view should be handled (if not already) by just doing nothing, and the nothing should be done in reaction to the synchronization of synapses on the graph view.

@dvcorreia dvcorreia marked this pull request as draft December 10, 2024 15:37
@dvcorreia dvcorreia marked this pull request as ready for review December 10, 2024 16:15
@dvcorreia dvcorreia requested a review from aranega December 10, 2024 16:15
Copy link
Member

@aranega aranega left a comment

Choose a reason for hiding this comment

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

All looks good overall @dvcorreia ! Thanks a lot 🙏 !

There is only 2/3 small modifications to do, but it's more about js/ts syntax, nothing else.

const { gray50, gray600, gray400B } = vars;
const CustomFormControlLabel = ({ label, tooltipTitle, helpText }) => {
const CustomFormControlLabel = ({ label, tooltipTitle, helpText, checked, onChange }: CustomFormControlLabel) => {
Copy link
Member

Choose a reason for hiding this comment

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

Indeed, I would move it somewhere else instead, if it's shared, it's weird to have it in the 3D viewer only.

@dvcorreia dvcorreia requested a review from aranega December 17, 2024 15:26
Copy link
Member

@aranega aranega left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the final modifications 🙏

@ddelpiano ddelpiano merged commit d1e9296 into develop Dec 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants