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: database upload modal rather than multiple upload buttons #164

Merged
merged 8 commits into from
Jul 20, 2023

Conversation

tonsV2
Copy link
Contributor

@tonsV2 tonsV2 commented Jul 14, 2023

The goal of this PR is to implement a global upload database button at the top of the page rather than a button associated with each group. The reason for this is that it's more aligned with the original design for the application.

There are two unresolved issues that I'd appreciate some support on

  • Loading indicator is displayed within form rather than on top of everything
  • The selected file isn't shown

@radnov radnov added the deploy Used to toggle deploying PR branches to the "feature" env. label Jul 17, 2023
Copy link
Collaborator

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

I don't really agree with these changes as they pollute the databases list component with code that does no have to do with listing. In my opinion we need one single component that:

  1. Renders a button that can be clicked to show the modal
  2. Contains the internal state for showing the modal
  3. Conditionally renders the modal

In React each component needs a single root "element", but our modal and button are siblings. For these cases we can use the React.Fragment (<></>) as root element. So you'd get something like this:

<>
    <Button />
    <Modal />    
</>

So the above is about architecture, but we also need to discuss naming. I understand your rename from -button to -modal given the current changes, but taking my suggestion into account this renaming no longer makes sense. And a good descriptive name is actually pretty difficult to come up with here. The component is a button that shows a modal, but it's not a DatabaseUploadModalButton or a DatabaseUploadButtonModal.

  1. I suppose DatabaseUploadButtonWithProgressModal would be accurate, but that's quite a long name...
  2. Another approach to naming we could use to end up with a shorter name would be to not describe what the component is but rather what the component does. This way we could opt for something like DatabaseUploader.

In general I prefer approach nr. 1 because it makes it easier to translate what you see in the code editor to what you see on your screen. I tend to only fallback to approach nr. 2 if the name would become ridiculously long, or if I simply can't come up with a good descriptive name at all.

Copy link
Contributor

@radnov radnov left a comment

Choose a reason for hiding this comment

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

The changes are deployed to https://im.feat-consistent-uoload-da.test.c.dhis2.org

I noticed one issue so far - clicking "Upload" without selecting any local files will succeed and upload an empty file named blob:
image

dependabot bot and others added 5 commits July 20, 2023 02:11
Bumps [@dhis2/ui](https://github.com/dhis2/ui/tree/HEAD/collections/ui) from 8.13.13 to 8.13.14.
- [Release notes](https://github.com/dhis2/ui/releases)
- [Changelog](https://github.com/dhis2/ui/blob/master/CHANGELOG.md)
- [Commits](https://github.com/dhis2/ui/commits/v8.13.14/collections/ui)

---
updated-dependencies:
- dependency-name: "@dhis2/ui"
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#171)

Bumps [@testing-library/jest-dom](https://github.com/testing-library/jest-dom) from 5.16.5 to 5.17.0.
- [Release notes](https://github.com/testing-library/jest-dom/releases)
- [Changelog](https://github.com/testing-library/jest-dom/blob/main/CHANGELOG.md)
- [Commits](testing-library/jest-dom@v5.16.5...v5.17.0)

---
updated-dependencies:
- dependency-name: "@testing-library/jest-dom"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@tonsV2 tonsV2 merged commit 3ec65fc into master Jul 20, 2023
2 checks passed
@tonsV2 tonsV2 deleted the feat/consistent_uoload_database_button branch July 20, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy Used to toggle deploying PR branches to the "feature" env.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants