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

refactor: [DHIS2-17750] replace material ui Card for Widget #3718

Merged
merged 3 commits into from
Aug 8, 2024

Conversation

simonadomnisoru
Copy link
Contributor

@simonadomnisoru simonadomnisoru commented Jul 18, 2024

DHIS2-17750

Tech summary

  • The material-ui Card was still used to display the program rules effects when adding a new event to a single event program. I replaced the Card for the Widgets

@simonadomnisoru simonadomnisoru requested a review from a team as a code owner July 18, 2024 08:35
Copy link
Contributor

@eirikhaugstulen eirikhaugstulen left a comment

Choose a reason for hiding this comment

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

Hey Simona! The updated version looks like a big improvement from the old cards and it will make our event pages seem much more modern.

Left some small comments on the PR itself, and then I have two questions that may or may not be relevant to this issue, but I'll post it here either way. Feel free to disregard if it should be separate tickets.

  1. Is this PR not relevant for the registration page? It looks like create new in program still uses the old widgets.
image
  1. Do you think we could change the styling so that the right column is not offset like it currently is?
image As you can see, the left and right column does not start at the same height, and I'm not sure why.

I think something like this would be an improvement:
image

All I did was to remove the marginTop: 10 from the mapping in withDataEntryOutput and in DataEntry.component.js, and then changed StickyOnScroll > stickyContainerAbsolute to something like this:

stickyContainerAbsolute: {
    position: 'static',
    '& > div > *:not(:first-child)': {
        marginTop: '10px',
    },
},

Let me know what you think. Thank you 🎉

</div>
);
const feedback = this.getItems();
return <WidgetFeedback feedback={feedback} emptyText={i18n.t('No feedback')} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we could follow the same emptyText pattern as we do in the EnrollmentPages?
i.e. No feedback for this event yet / No feedback for this enrollment yet, or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I implemented the same pattern now with the latest commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the feedback and indicator widgets are displaying all the time now, whereas in the enrollment pages and old widgets, they only rendered if there is a possibility that there will be text there. This means that if there are no program rule actions that can output anything there, they will be hidden all together. Do you think it's possible to keep this functionality?

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! I added back the functionality

@simonadomnisoru
Copy link
Contributor Author

Hey Simona! The updated version looks like a big improvement from the old cards and it will make our event pages seem much more modern.

Left some small comments on the PR itself, and then I have two questions that may or may not be relevant to this issue, but I'll post it here either way. Feel free to disregard if it should be separate tickets.

  1. Is this PR not relevant for the registration page? It looks like create new in program still uses the old widgets.
  2. Do you think we could change the styling so that the right column is not offset like it currently is?

Hey @eirik,
Regarding the widgets on the TEI registration page, the Material UI components are not used on that page, so this ticket is not relevant there. I implemented the rest of your suggestions.
Thank you for the feedback!

Copy link
Contributor

@eirikhaugstulen eirikhaugstulen left a comment

Choose a reason for hiding this comment

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

This is a major upgrade @simonadomnisoru! LGTM!

Copy link

github-actions bot commented Aug 7, 2024

Copy link

@geethaalwan geethaalwan left a comment

Choose a reason for hiding this comment

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

Tested successfully on 2.42 version

@simonadomnisoru simonadomnisoru merged commit 793da87 into master Aug 8, 2024
44 checks passed
@simonadomnisoru simonadomnisoru deleted the DHIS2-17750 branch August 8, 2024 13:49
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.76.1 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants