-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Disable Lift export when Frontier is empty #3440
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3440 +/- ##
==========================================
- Coverage 74.62% 74.61% -0.01%
==========================================
Files 285 285
Lines 10999 10999
Branches 1339 1338 -1
==========================================
- Hits 8208 8207 -1
Misses 2407 2407
- Partials 384 385 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
4b593ca
to
998c218
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @andracc)
src/components/ProjectExport/ExportButton.tsx
line 16 at r2 (raw file):
projectId: string; buttonProps?: ButtonProps & { "data-testid"?: string }; disabled?: boolean;
It looks like this new optional prop isn't been used.
src/components/ProjectExport/ExportButton.tsx
line 22 at r2 (raw file):
export default function ExportButton(props: ExportButtonProps): ReactElement { const dispatch = useAppDispatch(); const [exports, setExports] = useState<boolean>(false);
The <boolean>
can be dropped, since the type is implied from the default value.
src/components/ProjectExport/ExportButton.tsx
line 43 at r2 (raw file):
setExports(true); } });
Because setExports
is a function that takes a boolean, you should be able to condense this to
await isFrontierNonempty(props.propjectId).then(setExports);
(There's no performance concern for updating a state to what it already is, because rerenders are only triggered when states and props change.)
src/components/ProjectExport/ExportButton.tsx
line 53 at r2 (raw file):
<LoadingButton loading={loading} disabled={loading}
Since the LoadingButton
has a disabled
prop, probably better to update that with loading || !exports
than to conflict with it via the buttonProps
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @andracc)
src/components/ProjectExport/ExportButton.tsx
line 43 at r2 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Because
setExports
is a function that takes a boolean, you should be able to condense this toawait isFrontierNonempty(props.propjectId).then(setExports);
(There's no performance concern for updating a state to what it already is, because rerenders are only triggered when states and props change.)
One more possible simplification---since we're not doing any special error handling in this situation, the whole useEffect
can be:
useEffect(() => {
isFrontierNonempty(props.projectId).then(setExports);
}, [props.projectId]);
Also, we want the second argument: [props.projectId]
. That dependency array makes the useEffect
only re-run when the projectId
changes (e.g., when the user switches projects from within the project settings).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @andracc)
Resolves #3441
This change is