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

273 - Editing an experiment tag leads to blank page #314

Closed
wants to merge 92 commits into from

Conversation

Aiga115
Copy link
Contributor

@Aiga115 Aiga115 commented Oct 30, 2023

Closes #273

Implemented solution:
Displayed dialog window on card level, implemented on change function for each instance (name, desciption, tags), which changes the state, after calling the updateExperiment api we change the experiment then we call the function to call the experiments list again

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@Aiga115 Aiga115 requested a review from afonsobspinto October 30, 2023 13:12
Copy link
Member

@afonsobspinto afonsobspinto left a comment

Choose a reason for hiding this comment

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

Looks good but there are still a few problems:

  • the context menu changing position sometimes:
2023-10-30.17-29-01.mp4

If it's easier, I would be ok with the context menu being closed on modal close

  • There's currently no validation on the required fields

@afonsobspinto afonsobspinto changed the base branch from feature/243 to develop October 31, 2023 11:08
@Aiga115 Aiga115 requested a review from afonsobspinto October 31, 2023 15:51
return Response(status=status.HTTP_204_NO_CONTENT)
except Exception as e:
# This will return any error messages if the tag deletion fails
return Response({"error": str(e)}, status=status.HTTP_400_BAD_REQUEST)

Check warning

Code scanning / CodeQL

Information exposure through an exception

[Stack trace information](1) flows to this location and may be exposed to an external user.
Copy link
Member

@afonsobspinto afonsobspinto left a comment

Choose a reason for hiding this comment

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

Trying to edit a experiment twice (in sequence) doesn't seem to be updating the dashboard:
https://github.com/MetaCell/salk-interactive-atlas/assets/19196034/1d107be8-40ec-458b-9c26-df993a4727b2

const handleAction = async () => {
setIsLoading(true)
const errorsSet = await getValidationErrors()
console.log("errorset: ", errorsSet)
Copy link
Member

Choose a reason for hiding this comment

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

This can be deleted

Copy link
Member

@afonsobspinto afonsobspinto left a comment

Choose a reason for hiding this comment

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

I also noticed that your fix of the merge conflict is adding back some files that were removed purposely (namely data/residential_populations and objs files) which is not correct. Might be easier to branch from develop again and add your commits on top of that

freeSolo
limitTags={3}
renderTags={(value, getTagProps) =>
value.map((option, index) => (
<Chip key={index} style={{...tagsColorOptions[index]}} onDelete={() => console.log(option)} label={option} {...getTagProps({ index })} />
<Chip key={index} style={{ ...tagsColorOptions[index] }} onDelete={() => console.log(option)} label={option} {...getTagProps({ index })} />
Copy link
Member

Choose a reason for hiding this comment

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

This onDelete callback looks strange

@@ -89,7 +89,7 @@ export const App = (props: any) => {
{loading ? <Loader/> : (
<>
<ProtectedRoute exact={true} path="/">
<HomePage latestExperimentId={latestExperimentId} />
<HomePage latestExperimentId={latestExperimentId} onExperimentChange={(id: string) => setLatestExperimentId(id)} />
Copy link
Member

Choose a reason for hiding this comment

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

My guess is that the onExperimentChange callback is the reason why the bug I reported is happening.
When we change an experiment twitce, on the second change the id of the latest experiment doesn't change therefore there's no re-render of the dashboard.

@afonsobspinto
Copy link
Member

replaced by #317

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.

4 participants