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: make data sets sections collapsable #390

Merged
merged 6 commits into from
Sep 9, 2024

Conversation

flaminic
Copy link
Contributor

@flaminic flaminic commented Jul 8, 2024

Implements DHIS2-17507.
This PR allows for sections of a data set to be collapsible

Implementation details

We have decided that all sections should be collapsible by default, and this for now will not be a setting in the maintenance app

Collapsible is applied only when the " render as tabs" option is not turned on for the specific dataset

Background

We are aiming to add more form configuration options as part of an initiative to provide configurations natively to data entry forms to reduce the necessity for custom forms. Users are currently building custom forms as a workaround for shortcomings of the configuration options (ability to transpose, or customise a cell design) or implementation (such to avoid issues with RTL issues).

This is an RFC that describes the approach and the priorities for form configuration options. This is based on a thorough investigation by the functional design team for custom form use cases in real-life implementations. Based on that investigation, the ability to collapse sections were one of the main reasons people choose to go the custom forms route so we're tackling these first.

UI

This is a video of the feature:

Screen.Recording.2024-09-05.at.06.21.26.mov

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Jul 8, 2024

🚀 Deployed on https://pr-390--dhis2-data-entry.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify July 8, 2024 08:36 Inactive
@flaminic flaminic force-pushed the DHIS2-17507/collapse-sections branch from 9ac8ebe to 3abf159 Compare July 8, 2024 08:36
@dhis2-bot dhis2-bot temporarily deployed to netlify July 8, 2024 08:38 Inactive
@flaminic flaminic changed the title DHIS2 17507 Collapse sections feat: make data sets sections collapsable Jul 8, 2024
@flaminic flaminic requested a review from a team July 8, 2024 08:50
@flaminic flaminic force-pushed the DHIS2-17507/collapse-sections branch from 3abf159 to 1d7ab9d Compare July 9, 2024 07:55
@dhis2-bot dhis2-bot temporarily deployed to netlify July 9, 2024 07:57 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 9, 2024 13:27 Inactive
@flaminic flaminic force-pushed the DHIS2-17507/collapse-sections branch from 22063e2 to fd6bb57 Compare July 9, 2024 13:29
@dhis2-bot dhis2-bot temporarily deployed to netlify July 9, 2024 13:31 Inactive
Copy link
Member

@tomzemp tomzemp 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! 🪗

I did have a couple of general questions:

  • I'm not sure we'd want to disable the collapsing of the section if user has selected a section from context menu:
image I feel like there's no reason to enable the collapsing of a section if you - otherwise, I share the general opinion that Joe (or Kim/Karoline/Aarti) should maybe review to approve styling of collapsed sections and such

I'm not really sure about the use of the query parameter for allowing section toggling (I mean, I like it in terms of testing 😄, but not sure how it works for users to actually use in the app). Personally, I feel like this is a fairly non-intrusive UI change, such that maybe we could just it enable it by default (provided that sections are not display as tabs)? I feel like not having the query parameter also helps out in terms of the code organization in the sense that all of our query parameters would remain defined in use-context-selection.js (and related to context selection).

I can approve if you want to merge (the code looks good 🤩), but maybe we could just allow the toggling by default and then potentially update the app again when/if we decide that we want to toggle this feature based on a property of the data set?

{section.description && (
<div className={styles.description}>
{section.description ||
'Placeholder section description'}
Copy link
Member

Choose a reason for hiding this comment

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

this isn't your code, but it seems like we can just get rid of this, given that we already have the conditional check to display only if section.description (also, I assume we don't want to display generic text)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@flaminic
Copy link
Contributor Author

Looks good! 🪗

I did have a couple of general questions:

* I'm not sure we'd want to disable the collapsing of the section if user has selected a section from context menu:

image I feel like there's no reason to enable the collapsing of a section if you - otherwise, I share the general opinion that Joe (or Kim/Karoline/Aarti) should maybe review to approve styling of collapsed sections and such

I'm not really sure about the use of the query parameter for allowing section toggling (I mean, I like it in terms of testing 😄, but not sure how it works for users to actually use in the app). Personally, I feel like this is a fairly non-intrusive UI change, such that maybe we could just it enable it by default (provided that sections are not display as tabs)? I feel like not having the query parameter also helps out in terms of the code organization in the sense that all of our query parameters would remain defined in use-context-selection.js (and related to context selection).

I can approve if you want to merge (the code looks good 🤩), but maybe we could just allow the toggling by default and then potentially update the app again when/if we decide that we want to toggle this feature based on a property of the data set?

Hey, yes the feature toggle was added in order to hide the feature to users (since there are still unknowns on whether thins should be enabled by default and/or there should be a setting in the maintenance app etc). That decision is pending on a conversation scheduled for September. But i hear you, maybe there is no point in merging this until that is decided. I would say that we can leave this PR oper, close the JIra ticket and create a follow up ticket to finish this up. What do you think ?

@tomzemp
Copy link
Member

tomzemp commented Jul 18, 2024

. I would say that we can leave this PR oper, close the JIra ticket and create a follow up ticket to finish this up. What do you think ?

I think it's good to put the PR on hold until design team weighs in 👍. I'd probably just let them weigh in on the existing ticket, but sounds good also to close it and create a new one if that's easier for tracking progress.

@flaminic flaminic force-pushed the DHIS2-17507/collapse-sections branch from fd6bb57 to ad823a3 Compare September 4, 2024 15:36
@dhis2-bot dhis2-bot temporarily deployed to netlify September 4, 2024 15:38 Inactive
@cooper-joe
Copy link
Member

#392 covers my design/style suggestions.

Functionally, I think it would be ideal if sections retained their width when collapsed. Moving the collapse control to the start of the section heading makes this OK in the UI as the collapse control will not be hidden when the section is overflowing horizontally. Is this possible without too much of a hack? (I assume collapsed content is removed from the DOM, so we no longer have the width?)

@dhis2-bot dhis2-bot temporarily deployed to netlify September 4, 2024 17:49 Inactive
@flaminic flaminic force-pushed the DHIS2-17507/collapse-sections branch from 03f23d7 to 4af627b Compare September 4, 2024 17:59
@dhis2-bot dhis2-bot temporarily deployed to netlify September 4, 2024 18:01 Inactive
@flaminic
Copy link
Contributor Author

flaminic commented Sep 4, 2024

#392 covers my design/style suggestions.

Functionally, I think it would be ideal if sections retained their width when collapsed. Moving the collapse control to the start of the section heading makes this OK in the UI as the collapse control will not be hidden when the section is overflowing horizontally. Is this possible without too much of a hack? (I assume collapsed content is removed from the DOM, so we no longer have the width?)

Thanks Joe.I have made an attempt
here Is this is what you were thinking?

@cooper-joe
Copy link
Member

Thanks Joe.I have made an attempt
here Is this is what you were thinking?

Yes, that's what I had in mind. I think this works OK.

@dhis2-bot dhis2-bot temporarily deployed to netlify September 5, 2024 13:19 Inactive
@flaminic flaminic requested a review from tomzemp September 5, 2024 13:23
>
{collapsible &&
(showSectionContent ? (
<IconChevronUp16 color="white" />
Copy link
Member

Choose a reason for hiding this comment

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

we also have a constant value for white, in case we're worried that our design system is ever going to change DHIS2 white 😅 https://github.com/dhis2/ui/blob/master/constants/src/colors.js#L74

displayOptions: PropTypes.shape({
tabsDirection: PropTypes.oneOf(['vertical', 'horizontal']),
}),
displayOptions: PropTypes.string,
Copy link
Member

Choose a reason for hiding this comment

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

not related to this ticket, but it looks like we also have some logic for parsing display options here (

export const getDisplayOptions = (section) => {
), so we could probably just have one function for this parsing, and do it at the level. It looks like that getDisplayOptions gets used in the SectionFormSection child component.

Copy link
Member

@tomzemp tomzemp left a comment

Choose a reason for hiding this comment

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

Looks great to me 🚀!

I didn't know of visibilty: collapse 😎

@flaminic flaminic force-pushed the DHIS2-17507/collapse-sections branch from 9ee1c71 to 98217d2 Compare September 9, 2024 16:31
@dhis2-bot dhis2-bot temporarily deployed to netlify September 9, 2024 16:33 Inactive
@flaminic flaminic merged commit 1908b30 into master Sep 9, 2024
15 checks passed
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants