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(org unit tree): allow request customization #681

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Mohammer5
Copy link
Contributor

@Mohammer5 Mohammer5 commented Aug 3, 2021

This PR adds the possibility to customize the org unit tree requests. It also provides a possible style for our UI components to open up request customization:

A component with an app-runtime dependency exports two versions:

  1. A component that expects a function which can be used to retrieve the required information
  2. A component that wraps the above component and provides the respective functions

Advantages

  1. A third party app can use their own services to fetch additional data, which might be completely decoupled from the dhis2 instance
  2. App-runtime queries can be (more or less) static (more or less: Until queries accept multiple ids, the query must be generated)
  3. Allows to add any kind of potentially weird request behavior (e. g. sequential requests and whatnot)

Disadvantages

  1. Until the app-runtime provides a way to make the refetch function to throw when an error occurs, we need to use the engine's query method directly, which circumvents the use of the caching mechanism (as that one is build into the hook)
    • If we add an option to the hook that allows the behavior describe above, we can use the useDataQuery callback with that option and lazy: true
  2. The implementing app has to make sure that the response is an object
    • and the property on the response for the root data must use the org unit's ID as the key for the org unit data
    • and the property on the response for the individual org units must be organisationUnit

Neutral

  • Apps can re-use the default prop to preserve the original request behavior
    • Do we want to export that behavior instead?

Stories

As there were a lot of stories, I moved them into a folder called __stories__. In my opinion that makes it a lot easier to work on an individual story, but we'll have to talk (or at least agree) on the proposed structure

@Mohammer5 Mohammer5 force-pushed the Allow_request_customization_v2 branch 2 times, most recently from e228deb to ed4649d Compare August 3, 2021 09:35
@cypress
Copy link

cypress bot commented Aug 3, 2021



Test summary

512 0 0 0


Run details

Project ui
Status Passed
Commit 2687816
Started Aug 6, 2021 4:03 PM
Ended Aug 6, 2021 4:15 PM
Duration 11:54 💡
OS Linux Ubuntu - 20.04
Browser Electron 89

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@Mohammer5 Mohammer5 force-pushed the Allow_request_customization_v2 branch 2 times, most recently from 4d8a042 to 984da10 Compare August 3, 2021 12:43
@Mohammer5 Mohammer5 force-pushed the LIBS-203_Org_unit_tree_leaf_label_customization branch from 15a4024 to 3b7cdb1 Compare August 3, 2021 12:47
@Mohammer5 Mohammer5 force-pushed the Allow_request_customization_v2 branch 3 times, most recently from 1eb0a74 to 6e838f9 Compare August 4, 2021 12:16
@Mohammer5 Mohammer5 marked this pull request as ready for review August 6, 2021 09:19
Base automatically changed from LIBS-203_Org_unit_tree_leaf_label_customization to master August 6, 2021 12:28
@Mohammer5 Mohammer5 force-pushed the Allow_request_customization_v2 branch from ec9190c to bb23bc5 Compare August 6, 2021 12:37
@Mohammer5 Mohammer5 requested review from mediremi and varl August 6, 2021 12:54
@Mohammer5 Mohammer5 force-pushed the Allow_request_customization_v2 branch from 4568aec to 2687816 Compare August 6, 2021 15:04
* customize the node. In order to change the displayed node while keeping
* the existing functionality intact, you can re-use the original prop
* and overwrite the label property.
/** Renders the actual node label content for each leaf, can be used to
Copy link
Contributor

@mediremi mediremi Aug 10, 2021

Choose a reason for hiding this comment

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

JSDocs was replaced with react-docgen in #688, so this prop's description needs to be updated to:

    /** Renders the actual node component for each leaf, can be used to
     * customize the node. The default function just returns the node's
     * displayName
     * 
     * Shape of the object passed to the callback:
     * ```
     * {
     *    label: string,
     *    node: {
     *      displayName: string,
     *      id: string,
     *      // Only provided once `loading` is false
     *      path?: string,
     *      // Only provided once `loading` is false
     *      children?: Array.<{
     *        id: string,
     *        path: string,
     *        displayName: string
     *      }>
     *    },
     *    loading: boolean,
     *    error: string,
     *    open: boolean,
     *    selected: string[],
     *    singleSelection: boolean,
     *    disableSelection: boolean,
     *    // If the request is being customized, then all responses except the org
     *    // unit's response data will be included in this object
     *    additional?: object
     * }
     * ``` */
    renderNodeLabel: propTypes.func,

@mediremi mediremi marked this pull request as draft August 10, 2021 13:16
@mediremi
Copy link
Contributor

I'm converting this PR to a draft as after discussing with @HendrikThePendric we've agreed to implement these changes in the https://github.com/dhis2/approval-app repo.

@stale
Copy link

stale bot commented Mar 2, 2022

Hi!

Due to a lack of activity on this issue over time (180 days) it seems to be stale. If still relevant, please provide information that moves it forward, e.g. additional information, a pull request with suggested changes, or a reason to keep it open.

Any activity will keep it open, otherwise it will be closed automatically in 30 days. Thanks! 🤖

@stale stale bot added the stale No activity for a while label Mar 2, 2022
@Mohammer5
Copy link
Contributor Author

keep it open

@stale stale bot removed the stale No activity for a while label Mar 3, 2022
@stale
Copy link

stale bot commented Apr 19, 2023

Hi!

Due to a lack of activity on this issue over time (180 days) it seems to be stale. If still relevant, please provide information that moves it forward, e.g. additional information, a pull request with suggested changes, or a reason to keep it open.

Any activity will keep it open, otherwise it will be closed automatically in 30 days. Thanks! 🤖

@stale stale bot added the stale No activity for a while label Apr 19, 2023
@Mohammer5
Copy link
Contributor Author

@HendrikThePendric is this PR of any relevance to you?

@stale stale bot removed the stale No activity for a while label Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants