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

feat: data set required and setup fields #474

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

flaminic
Copy link
Contributor

No description provided.

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for dhis2-maintenance-app-beta ready!

Name Link
🔨 Latest commit 0e4b907
🔍 Latest deploy log https://app.netlify.com/sites/dhis2-maintenance-app-beta/deploys/676926d9c0f100000854cd4c
😎 Deploy Preview https://deploy-preview-474.maintenance-app-beta.netlify.dhis2.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -30,8 +30,8 @@ export const CategoryComboSelect = forwardRef(function CategoryComboSelect(
required={required}
invalid={invalid}
disabled={disabled}
useInitialOptionQuery={useInitialOptionQuery}
useOptionsQuery={useOptionsQuery}
useInitialOptionQuery={useInitialCategoryComboQuery}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are just renaming

onComplete({ value, label })
},
})
return useDataQuery<InitialCategoryComboQueryResult>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

again only renaming in this file

@@ -39,7 +39,7 @@ export const BaseModelSingleSelect = <
const { allModelsMap, allSingleSelectOptions } = useMemo(() => {
const allModelsMap = new Map(available.map((o) => [o.id, o]))
// due to pagination, the selected model might not be in the available list, so add it
if (selected && !allModelsMap.get(selected.id)) {
if (selected && selected.id && !allModelsMap.get(selected.id)) {
Copy link
Member

Choose a reason for hiding this comment

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

How can the selected be truthy but not contain an id with the types here? This is more protective and it can stay, but it shouldn't really happen with correct typing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah i added it cause i messed everything up by sending selected=undefined. But you are right, prob if i fixed teh types before that it would not have happened

@@ -10,7 +10,7 @@ type OptionSetQueryResult = {
}
}

const CATEGORY_COMBOS_QUERY = {
const OPTION_SETS_QUERY = {
Copy link
Member

Choose a reason for hiding this comment

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

🙃

Comment on lines 11 to 21
const CATEGORY_COMBOS_QUERY = {
resource: 'categoryCombos',
params: {
filter: ['dataDimensionType:eq:ATTRIBUTE'],
},
}

const DEFAULT_CATEGORY_SELECT_OPTION = {
id: DEFAULT_CATEGORY_COMBO.id,
displayName: DEFAULT_CATEGORY_COMBO.displayName,
}
Copy link
Member

Choose a reason for hiding this comment

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

These could easily be statically defined outside render?

}

return (
<Field
Copy link
Member

Choose a reason for hiding this comment

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

You could use ModelSingleSelectField here?

// categoryCombo: z.object({ id: z.string() }),
categoryCombo: z
.object({ id: z.string() })
.default({ id: DEFAULT_CATEGORY_COMBO.id }),
Copy link
Member

@Birkbjo Birkbjo Dec 19, 2024

Choose a reason for hiding this comment

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

Could we change this so we can default to the whole of default? Eg so we don't have to fetch displayName?

categoryCombo: z.object({ id: z.string(), displayName: z.string()}).default({ ...fromdefaultcatcombo})

Maybe we could take the transformed values into account when before fetching selectedWithoutId in ModelSingleSelect instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for your first suggestion as i think really transform is a function that should run once the values are fetched. We could add a default value prop but maybe for now giving the default prop values in their completed form is less complex

}}
transform={(catCombos) => [
...catCombos,
DEFAULT_CATEGORY_SELECT_OPTION,
Copy link
Member

Choose a reason for hiding this comment

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

This should be the first option to match old app behaviour

import { useBoundResourceQueryFn } from '../../../lib/query/useBoundQueryFn'

export type PeriodTypes = { periodTypes: [{ name: string }] }
const periodTypesToTranslatedValues: Record<string, string> = {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer for these to be defined in translatedModelConstants. Then use getConstantTranslation to get the value (which will take care of the fallback).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah amazing thanks. Forgot we had already something

@flaminic flaminic force-pushed the DHIS2-18538/data-set-required-fields branch from a3a9ae5 to 364e0ae Compare December 23, 2024 08:59
@flaminic flaminic force-pushed the DHIS2-18538/data-set-required-fields branch from 364e0ae to 0e4b907 Compare December 23, 2024 09:01
@flaminic flaminic requested a review from Birkbjo December 23, 2024 09:08
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