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.

Sign up for GitHub

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

Jump to bottom

feat: add DHIS2 Sharing Dialog (TECH-274) #24

Merged
merged 50 commits into from
Aug 24, 2021
Merged

feat: add DHIS2 Sharing Dialog (TECH-274) #24

merged 50 commits into from
Aug 24, 2021

Conversation

amcgee
Copy link
Member

@amcgee amcgee commented Apr 16, 2020 •
edited by edoardo
Loading

This is the implementation of the DHIS2 Sharing Dialog (previously d2-ui-sharing-dialog)

HUGE credit to @cooper-joe who implemented the first React version of this :-D (now outdated)

Here is the design spec.

Screenshot 2020-11-23 at 10 08 42

Cascading sharing for dashboard

Design spec

Screenshots:
Screenshot 2021-07-16 at 16 51 06
Screenshot 2021-08-09 at 10 56 02

TODO (help wanted):

  • Basic UI layout
  • Add / edit / delete permissions
  • Live state and diffing of current state vs initial state for Save button enablement
  • Fixed-width select input
  • Support adding users and groups instead of just users
  • Validate and finalize the sharingSettings object structure
  • Dynamically load users and groups for auto-complete in new permission
  • Public icon and "My organization" text (should this be dynamic @cooper-joe?)
  • Proper ordering of users and groups (should the groups always come first?)
  • Group icon
  • User avatars (name initials as per design)
  • Maybe use the noun Permission for a sharing grant to a particular user or group?
  • Clean up code, add PropTypes
  • Add support for data sharing settings (only applies to some objects)
  • Optimise for small screens
  • Complete dashboard specific part for cascading sharing once the backend for it is ready

@amcgee amcgee requested a review from a team as a code owner April 16, 2020 14:46
@amcgee
Copy link
Member Author

amcgee commented Apr 16, 2020

This was supposed to be a draft... apologies

@amcgee amcgee changed the title feat: Add DHIS2 Sharing Dialog (TECH-274) WIP: Add DHIS2 Sharing Dialog (TECH-274) Apr 16, 2020
@amcgee amcgee marked this pull request as draft April 16, 2020 14:52
@amcgee amcgee mentioned this pull request Apr 16, 2020
Closed
@amcgee amcgee changed the title WIP: Add DHIS2 Sharing Dialog (TECH-274) feat: Add DHIS2 Sharing Dialog (TECH-274) Apr 18, 2020
@edoardo
Copy link
Member

edoardo commented May 7, 2020

I added the search for users/groups while typing in the search field.
A list is populated with the matching results and each item can be clicked to be added to the list of permissions.
The removal of a permission also works now for both users and groups.
debounce doesn't work properly yet.
propTypes have been added where missing.

The Autocomplete component should be generic enough so that when we have a new use case for it, it can be moved as a standalone widget and imported wherever needed.
I'm not convinced about the name, I'm open for suggestions.
Also we can discuss and decide how flexible we want it, right now the list with the search results is built-in.

@cypress
Copy link

cypress bot commented May 7, 2020 •
edited
Loading


Test summary

512 • 0 • 0 • 0

Run details
Project ui
Status Passed
Commit 70f2196
Started Aug 24, 2021 8:27 AM
Ended Aug 24, 2021 8:34 AM
Duration 07:42 💡
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

HendrikThePendric
HendrikThePendric reviewed May 7, 2020
View reviewed changes
Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is looking good already. A small point reg. i18n that came up yesterday and is relevant for this component too:

  • Apps will only know the user-locale after the app bundle has been loaded and some initial requests have been done.
  • Prior to this the default locale, i.e. en will be used.
  • As a result, all calls to i18n.t() that are executed directly in a module, will alway use the default locale
  • I'll place some comments in the code to illustrate points where this goes wrong

(actually it turned out there was only one occurrence of this)

Base automatically changed from alpha to master May 28, 2020 11:30
@edoardo edoardo changed the title feat: Add DHIS2 Sharing Dialog (TECH-274) feat: add DHIS2 Sharing Dialog (TECH-274) Oct 13, 2020
@edoardo edoardo mentioned this pull request Oct 20, 2020
Merged
3 tasks
@HendrikThePendric
Copy link
Contributor

My apologies @edoardo: in my last comment #24 (comment) I pointed out a problem with the i18n strings, but I failed to point out we already have a small translate helper for this (could be that wasn't the case when I last reviewed). See a usage example here:

ui/packages/widgets/src/FileInputField/FileInputField.js

Lines 85 to 103 in c1fa323

<FileList>
{children ? (
children
) : (
<FileListPlaceholder>
{translate(placeholder)}
</FileListPlaceholder>
)}
</FileList>
</Field>
)
FileInputField.defaultProps = {
accept: '*',
dataTest: 'dhis2-uiwidgets-fileinputfield',
buttonLabel: () => i18n.t('Upload a file'),
placeholder: () => i18n.t('No file uploaded yet'),
}

Also, the code is looking very good already and I noticed only one TODO in a comment. This PR is still in draft, so I haven't done a full review. What is still needed to take this out of draft status?

@edoardo edoardo changed the base branch from master to next December 14, 2020 08:23
Base automatically changed from next to master December 17, 2020 13:37
@cooper-joe
Copy link
Member

Note that the original sketch posted here doesn't include the interaction if a sharing dialog needs to handle both data and metadata. I'm not sure that is included in this PR, but adding this mockup here for information.
image

@edoardo
Copy link
Member

edoardo commented Jan 14, 2021

@cooper-joe the data part is not implemented in this PR.

Mohammer5
Mohammer5 reviewed Mar 12, 2021
View reviewed changes
Copy link
Contributor

@Mohammer5 Mohammer5 left a comment

Choose a reason for hiding this comment

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

Component appears to be quite generic, we could use it in all apps that allow setting sharing options, right?

Left some minor comments. When I figured out that this is still a draft, I stopped looking at details. Variable names could be improved a bit.

packages/widgets/src/SharingDialog/SharingDialog.js Outdated Show resolved Hide resolved
packages/widgets/src/SharingDialog/SharingDialog.js Outdated Show resolved Hide resolved
packages/widgets/src/SharingDialog/sharingConstants.js Outdated Show resolved Hide resolved
@edoardo
Copy link
Member

edoardo commented Mar 12, 2021

The component is supposed to be generic and reusable by different apps.
However, this PR does not include the implementation for the Data part of the sharing, which is used in Maintenance app.
The plan was to adopt this widget in the new FileMenu analytics apps are now using, there we only need the Metadata part of the sharing.

@Mohammer5
Copy link
Contributor

I'm not sure what we'd have to change in terms of props (e. g. initialSharingSettings) to make it work with data as well.
Is this component already built in a way that we just have to add information to that structure?

@amcgee
Copy link
Member Author

amcgee commented Mar 14, 2021

@edoardo do you think we could release this with metadata support only and then add data later? I think it would be really great to get this into use. If it looks like this might be close to ready I can take a look and "review" as well (though I can't actually review since I authored the original PR)

@Mohammer5 were you asking about initialSharingSettings etc. to determine if a breaking change would be needed to add data sharing later?

@Mohammer5
Copy link
Contributor

@Mohammer5 were you asking about initialSharingSettings etc. to determine if a breaking change would be needed to add data sharing later?

@amcgee yes

@edoardo
Copy link
Member

edoardo commented Mar 16, 2021

The plan was to release this with metadata support only.
Since it didn't make it for 2.36 I'm wondering if we should complete it before we publish it.
There is another relatively important issue, currently it's not responsive and doesn't work in small screens.
Adopting it as it is would be a problem in dashboards app.

As for the breaking change, I don't think it would necessary be the case, I think the changes can be all internal, keeping the same props.

@Mohammer5
Copy link
Contributor

Do we have a design or specs for what it should look like on smaller screens?

@edoardo
Copy link
Member

edoardo commented Mar 16, 2021

Do we have a design or specs for what it should look like on smaller screens?

Not that I know. @cooper-joe ?

@varl varl changed the base branch from master to alpha August 23, 2021 11:43
@dhis2-bot
Copy link
Contributor

dhis2-bot commented Aug 23, 2021 •
edited
Loading

🚀 Deployed on https://pr-24--dhis2-ui.netlify.app

varl
varl reviewed Aug 23, 2021
View reviewed changes
collections/widgets/src/index.js
@@ -23,3 +23,4 @@ export {
DataTableRow,
DataTableToolbar,
} from '@dhis2-ui/table'
// TODO export { SharingDialog } from '@dhis2-ui/sharing-dialog'
Copy link
Contributor

Choose a reason for hiding this comment

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

Once ready, this line should be added to the @dhis2/ui package, but we can release it in alpha without exposing it.

Suggested change
// TODO export { SharingDialog } from '@dhis2-ui/sharing-dialog'

components/sharing-dialog/src/access-select.js Outdated Show resolved Hide resolved
components/sharing-dialog/src/access-select.js Outdated Show resolved Hide resolved
@varl varl marked this pull request as ready for review August 24, 2021 08:16
@varl varl merged commit 6bdb92d into alpha Aug 24, 2021
@varl varl deleted the feat/sharing-dialog branch August 24, 2021 08:38
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 6.19.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@edoardo edoardo mentioned this pull request Aug 24, 2021
Merged
1 task
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 6.20.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 6.20.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
Reviewers

@edoardo edoardo edoardo left review comments

@varl varl varl left review comments

@HendrikThePendric HendrikThePendric HendrikThePendric left review comments

@Mohammer5 Mohammer5 Mohammer5 left review comments

Assignees
No one assigned
Labels
released on @alpha released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@amcgee @edoardo @HendrikThePendric @cooper-joe @Mohammer5 @dhis2-bot @varl