Skip to content

Fix: Adjustments to new layout based on feedback#2011

Merged
mykalmax merged 6 commits intomainfrom
fix-remove-tags
Jan 25, 2024
Merged

Fix: Adjustments to new layout based on feedback#2011
mykalmax merged 6 commits intomainfrom
fix-remove-tags

Conversation

@mykalmax
Copy link
Contributor

No description provided.

@mykalmax mykalmax force-pushed the fix-remove-tags branch 2 times, most recently from 2a3337f to a6e5d8f Compare January 24, 2024 23:24
@mykalmax mykalmax force-pushed the fix-remove-tags branch 2 times, most recently from bc12479 to 9fd4bdf Compare January 25, 2024 06:15
@mykalmax mykalmax requested a review from vchan January 25, 2024 07:17

void planRun()
}
}, [models])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to have the dependency be models.size instead of models? Or it doesn't matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

having models.size may not catch when some value that models map holds has changed

}
}, [models])

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very similar to the effect hook above it. Is that intentional?

if (isNotNil(value)) {
setPlanAction(new ModelPlanAction({ value }))
}
// This inconsistency can happen when we recived flag from the backend
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This inconsistency can happen when we recived flag from the backend
// This inconsistency can happen when we receive flag from the backend



class Mode(str, enum.Enum):
DEFAULT = "default" # Allow all modules
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: How about ALL instead of DEFAULT?

@mykalmax mykalmax merged commit 499fea7 into main Jan 25, 2024
@mykalmax mykalmax deleted the fix-remove-tags branch January 25, 2024 21:01
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.

2 participants