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) O3-3252: Form engine UI improvements #282

Merged
merged 2 commits into from
May 21, 2024
Merged

(feat) O3-3252: Form engine UI improvements #282

merged 2 commits into from
May 21, 2024

Conversation

denniskigen
Copy link
Member

@denniskigen denniskigen commented May 20, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This PR applies various fixes to the form engine UI, including:

  • Fixing the form layout in the tablet viewport
  • Making several tweaks to align with the available designs here and here.

Screenshots

Before

Tablet

CleanShot 2024-05-20 at 9  40 29 2@2x

Desktop (maximized) - the sidebar doesn't quite match the designs

CleanShot 2024-05-20 at 11  10 05@2x

After

Tablet

CleanShot 2024-05-20 at 10  58 28@2x

Desktop

CleanShot 2024-05-20 at 10  57 49@2x

Desktop (maximized) - the sidebar aligned with the designs

CleanShot 2024-05-20 at 10  58 11@2x

Related Issue

https://openmrs.atlassian.net/browse/O3-3252

Other

Copy link

github-actions bot commented May 20, 2024

Size Change: +385 B (+0.04%)

Total Size: 1.05 MB

ℹ️ View Unchanged
Filename Size Change
dist/184.js 11.2 kB 0 B
dist/225.js 2.56 kB 0 B
dist/29.js 157 kB 0 B
dist/300.js 682 B 0 B
dist/31.js 4.9 kB 0 B
dist/318.js 2.42 kB 0 B
dist/327.js 1.71 kB 0 B
dist/335.js 955 B 0 B
dist/353.js 3.02 kB 0 B
dist/368.js 240 kB +133 B (+0.06%)
dist/371.js 71.5 kB 0 B
dist/41.js 3.36 kB 0 B
dist/436.js 752 B 0 B
dist/540.js 2.64 kB 0 B
dist/55.js 2.14 kB 0 B
dist/635.js 14.3 kB 0 B
dist/779.js 203 kB 0 B
dist/8.js 3.67 kB 0 B
dist/942.js 482 B 0 B
dist/979.js 6.85 kB 0 B
dist/99.js 690 B 0 B
dist/993.js 3.08 kB 0 B
dist/998.js 15.5 kB 0 B
dist/main.js 298 kB +252 B (+0.08%)
dist/openmrs-form-engine-lib.js 3.76 kB 0 B

compressed-size-action

@ibacher
Copy link
Member

ibacher commented May 20, 2024

Definitely an improvement! One small comment: the whole point of rendering a form in a slightly larger workspace on desktops was to give us space to render the navigation stuff, but it looks like that's only available in the full-screen variant?

@denniskigen
Copy link
Member Author

Yeah, I'd love to hear @samuelmale's thoughts on that, Ian.

@pirupius
Copy link
Member

Thanks @denniskigen for this clean up. I've noticed a discrepancy in the background colors with the soft white cutting off midway the last field (which in this case is the select field) could there be an extra margin or padding causing that?

Screenshot 2024-05-21 at 09 57 22

Copy link
Member

@pirupius pirupius left a comment

Choose a reason for hiding this comment

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

This might not be related to this PR but there seems to be a missing label. Wasn't this resolved?

@denniskigen
Copy link
Member Author

denniskigen commented May 21, 2024

About the padding cutting off fields

That's right, there's a bit of padding around the button set on the tablet viewport. For context, see this Zeplin screen of the Medication Order form. The designs also envision the sidebar appearing when in the tablet viewport, which is currently not the case. If we go in that direction, we wouldn't need to show the button set at the bottom of the page. That's what Ian is suggesting above.

If we decide not to show the sidebar on tablet, one way to mitigate the issue you raised could be to apply a top border to the button set container.

This might not be related to this PR but there seems to be a missing label. Wasn't this resolved?

This issue was resolved. I haven't bumped the Core tooling in my local copy of the Patient Chart, which is why you see some labels fail to load.

Copy link
Member

@samuelmale samuelmale left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @denniskigen for the UI revamp 🎉

@denniskigen
Copy link
Member Author

Please don't merge yet, @samuelmale. I have a commit on the way.

@denniskigen
Copy link
Member Author

Sorry for the additional scope in the latest commit, hopefully it all makes sense.

@samuelmale, About @ibacher's question, do you know whether a ticket exists to handle it. Is it something you've considered?

@samuelmale
Copy link
Member

samuelmale commented May 21, 2024

do you know whether a ticket exists to handle it. Is it something you've considered?

@denniskigen not to my knowledge.

@denniskigen denniskigen merged commit a9cca75 into main May 21, 2024
4 checks passed
@denniskigen denniskigen deleted the feat/O3-3252 branch May 21, 2024 11:26
@denniskigen denniskigen changed the title (feat) Form engine UI improvements (feat) O3-3252: Form engine UI improvements May 21, 2024
@denniskigen
Copy link
Member Author

I've filed this issue to explore the feasibility of rendering the form navigation menu in the tablet and minimized desktop views.

vasharma05 pushed a commit that referenced this pull request May 27, 2024
* (feat) Form engine UI improvements

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

Successfully merging this pull request may close these issues.

4 participants