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

fix: add scroll for workflows [DHIS2-15415] #276

Merged
merged 5 commits into from
Aug 8, 2023

Conversation

tomzemp
Copy link
Member

@tomzemp tomzemp commented Jun 26, 2023

Adds overflow-y and sets max height for workflow menu. See https://dhis2.atlassian.net/jira/software/c/projects/DHIS2/issues/DHIS2-15415. The other selectors (for period and organisation unit) have differing fixed max-heights, so I just somewhat randomly went with .7vh here 🫤.

Note:
There were a number of tests that were failing when I went to make this update. The tests aren't failing because of the changes I've made (the stylistic changes don't affect anything, particularly for our testing where admin user only has 1 workflow), but I couldn't get a successful run without addressing these:

  • a jest test was failing because the expected text "Approved 2 years ago" was out of date (the test was comparing against static date of 2020-01-01, so now 3 years ago)
  • cypress tests were failing because the cypress tests has user select March of the current year (currently March 2023). The fixtures previously generated were for March 2022, so when these ran in CI, the tests failed as the request/response for 2023 was missing. Also the CI was set up to run with v37 and v38, but v37 is no longer maintained, so I updated this such that they run against v38 and v39 (Maybe we actually want v40 and v41 at this point?). I had a number of issues updating these fixtures locally (my local runs timed out, so I had to increase the default timeout in order to generate the fixtures), and then I had to point to debug directly when generating these (the package.json file is assuming that v38 is at localhost:8081 for example) (I'm just including these notes here as I think maybe we should add a ticket to update the cypress set up more generally for this app, but I've done some minimum for now to get the PR to pass)

Before:
image

After:
image

@tomzemp tomzemp requested review from a team as code owners June 26, 2023 15:02
@dhis2-bot
Copy link
Contributor

🚀 Deployed on https://pr-276--dhis2-data-approval.netlify.app

const now = new Date()
const yearTwoYearsAgo = now.getFullYear() - 2
return () =>
jest.requireActual('moment')(`${yearTwoYearsAgo}-01-01T00:00:00.000Z`)
Copy link
Member Author

Choose a reason for hiding this comment

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

There is a test that expects output of Approved by Hendrik 2 years ago. However, since the date here was fixed as 01-01-01T00:00:00.000 when I ran the test now in 2023, I got the output of Approved by Hendrik 3 years ago.

This update just sets it so the date will be 1 January two years prior to the current year, so that the output text should always be Approved by Hendrik 2 years ago.

@@ -3,3 +3,7 @@
color: var(--colors-grey700);
padding: var(--spacers-dp16) var(--spacers-dp8) var(--spacers-dp24);
}
.menu {
max-height: 70vh;
Copy link
Member Author

Choose a reason for hiding this comment

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

as noted in the PR description, this is just a random value I chose as the other two drop downs were already different.

Copy link
Member

@Birkbjo Birkbjo left a comment

Choose a reason for hiding this comment

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

LGTM.

Sorry that you had to deal with these test-issues. However, I have to admit I chuckled a bit when I read the two years ago problem.

Copy link

@helladawit helladawit 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.41 version

@tomzemp tomzemp merged commit cd8aa4e into master Aug 8, 2023
@tomzemp tomzemp deleted the DHIS2-15415/add-scroll-for-workflows branch August 8, 2023 11:19
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.0.8 🎉

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