(feat) O3-5027: Implement beautiful configurable login page #1460
Conversation
|
|
||
| case 'gradient': | ||
| if (background.value) { | ||
| style.background = background.value; |
There was a problem hiding this comment.
Hello @UjjawalPrabhat , We need Loginbackground to be configurable, i went ahead and tested your code .Check this out.
loginbackground.webm These changes override the default login image.
There was a problem hiding this comment.
Hey @sherrif10, the Logo change config you're referring to wasn't implemented by me. It was there from before and therefore the logo is editable. If you're referring to background change then that is available in a separate section called as background.
Do you want me to make the Logo constant?
Also please let me know if there's any other changes or enhancements required.
There was a problem hiding this comment.
We need the login page to look like this . Share the vedio of your changes before and after
There was a problem hiding this comment.
@sherrif10 could you send the images used in this the login page?
There was a problem hiding this comment.
Hello @UjjawalPrabhat , We want to come up with something like that in the image i shared, FYI , Login page is already configurable, the only changes needed is to make login page appear like that.
There was a problem hiding this comment.
@sherrif10 I'm on it. I was talking about the images used like the 3 doctor's and logo image used in the login page. It'd be easier for me to configure then
There was a problem hiding this comment.
Yes it needs to be configurable and when the configuration is set to default, Default OMRS login page should appear, Then you should consider making it appear left or right on the login page.
|
Watching |
Hey @sherrif10, @tendomart |
| @@ -0,0 +1,88 @@ | |||
| import React, { useState, useCallback, useMemo } from 'react'; | |||
There was a problem hiding this comment.
Lets only focus on login page for now, This can come back in separate ticket cc @denniskigen
There was a problem hiding this comment.
I agree with @sherrif10 we should first focus on having an image side by side with the login form he shared above
hey @UjjawalPrabhat did you have a chance to look at how it was implemented at https://github.com/METS-Programme/esm-ugandaemr-login ?
I don't think you really need to come up with something completely new
There was a problem hiding this comment.
@denniskigen can you please help us approve the workflows here
There was a problem hiding this comment.
I agree with @sherrif10 we should first focus on having an image side by side with the login form he shared above
hey @UjjawalPrabhat did you have a chance to look at how it was implemented at https://github.com/METS-Programme/esm-ugandaemr-login ? I don't think you really need to come up with something completely new
@tendomart Thanks for this. I actually implemented it from scratch and then saw this when i was about to commit. But I made all the required changes including login page switch b/w uganda-emr and default, configurable system-messages and background.
@sherrif10 Please review my changes and let me know if any further changes required.
| import styles from './background-wrapper.scss'; | ||
|
|
||
| // Import Uganda EMR background for default usage | ||
| const getUgandaEmrBackgroundUrl = () => { |
There was a problem hiding this comment.
This is not supposed to be called Ugandaemr. its supposed to be generic. You are simply picking what was done in uganda emr and do it this side as simple as that. Like i had said. We only need the design, on the implementation side, An implementor simply needs to configure a background image and The login page , What is left is designing the page,
There was a problem hiding this comment.
Hello @UjjawalPrabhat Also remember for all this to happen out of the box. You need to map this image on the server either via dockerFile in openmrs distro to let docker pick the path such that immediately you start openmrs-esm-login -app, it can pick that Path and renders it via config that we made in esm-login-app. cc @reagan-meant @ibacher
There was a problem hiding this comment.
Ideally, the user can just supply a (relative) URL in configuration and that will be used to display the image. We should not be doing anything to fetch the image, especially using require() and the like, which only work if the image is part of the login app.
| } | ||
| } | ||
|
|
||
| // Uganda EMR specific background styles (from original app) |
There was a problem hiding this comment.
Please remove those openmrs images and ugandaEMR image. Those are supposed to be handled by the server.
|
This PR has way too many changes not related to the Login page. Can we please remove those changes from this PR completely? PRs that are larger than they need to be are usually impossible to merge in because they will break too many things. |
| import styles from './background-wrapper.scss'; | ||
|
|
||
| // Import Uganda EMR background for default usage | ||
| const getUgandaEmrBackgroundUrl = () => { |
There was a problem hiding this comment.
Ideally, the user can just supply a (relative) URL in configuration and that will be used to display the image. We should not be doing anything to fetch the image, especially using require() and the like, which only work if the image is part of the login app.
UjjawalPrabhat
force-pushed
the
feature/configurable-login-page
branch
from
0cd6e18 to
4f67b63
Compare
|
@ibacher @sherrif10 @tendomart Fixed the scope issues! They're all just in the login app now. Removed the framework stuff and simplified the image loading like you asked - now it just uses URLs directly instead of bundling assets. All the login customization still works (backgrounds, layouts, branding, etc.) but it's much cleaner implementation. Ready for another look when you get a chance 👍 |
| left: 0; | ||
| } | ||
|
|
||
| // Configurable branding styles |
There was a problem hiding this comment.
Remove this comments, Then feel free to share the latest vedio of this changea
There was a problem hiding this comment.
@sherrif10 you want me to remove all comments throughout my code changes or just this one?
There was a problem hiding this comment.
Remove this comments, Then feel free to share the latest vedio of this changea
@sherrif10 done
Significantly reduced the number of changed files based on @ibacher's comments. Removed all framework modifications and simplified the background image handling to use direct URLs. All the configurable login features are still there - just implemented more cleanly now. Should be much easier to review and merge.
UjjawalPrabhat
force-pushed
the
feature/configurable-login-page
branch
from
4b0f4a3 to
5fb29bd
Compare
ibacher
left a comment
ibacher
left a comment
There was a problem hiding this comment.
A few notes almost entirely on the configuration. Note that the idea here is to allow some simple, light customization easily. We shouldn't have nearly so many CSS-specific properties exposed as configuration options.
Let's focus on primarily supporting single color backgrounds and background images and try to minimize additional properties. We can add them as and when they become relevant.
| _description: | ||
| 'Whether to show the password field on a separate screen. If false, the password field will be shown on the same screen.', | ||
| }, | ||
| layout: { |
There was a problem hiding this comment.
This should have _type: Type.Object. It would also be a good place to include an overview.
| columnPosition: { | ||
| _type: Type.String, | ||
| _default: 'center', | ||
| _description: | ||
| 'Position of the login card on the screen. ' + | ||
| 'For default layout: positions the card left/center/right on the screen. ' + | ||
| 'For split-screen layout: positions the card and shows background image on the opposite side.', | ||
| _validators: [validators.oneOf(['left', 'center', 'right'])], | ||
| }, |
There was a problem hiding this comment.
What does the "opposite" side mean if this is set to "center"? Do we need two options here? Isn't "type" basically derivable from "columnPosition"?
Finally, we should rename "columnPosition" to "loginFormPosition" or something. Please change the word "card" here to "form". The login form is not really a card in any meaningful sense.
| showLogo: { | ||
| _type: Type.Boolean, | ||
| _default: true, | ||
| _description: 'Whether to show the logo on the login page.', | ||
| }, |
| _description: 'Whether to show the footer on the login page.', | ||
| }, | ||
| }, | ||
| card: { |
| backgroundColor: { | ||
| _type: Type.String, | ||
| _default: '', | ||
| _description: 'Custom background color for the login card. Leave empty to use theme default.', | ||
| }, |
There was a problem hiding this comment.
This probably deserve either it's own type or at least some kind of validation...
| title: { | ||
| _type: Type.String, | ||
| _default: '', | ||
| _description: 'Custom title to display above the login form (e.g., "Welcome to MyClinic").', | ||
| }, | ||
| subtitle: { | ||
| _type: Type.String, | ||
| _default: '', | ||
| _description: 'Custom subtitle to display below the title.', | ||
| }, |
There was a problem hiding this comment.
These should support being specified as translation keys and not just simple strings.
There was a problem hiding this comment.
This goes for basically every text element.
| contactEmail: { | ||
| _type: Type.String, | ||
| _default: '', | ||
| _description: 'Contact email to display for support.', | ||
| }, |
There was a problem hiding this comment.
Why is this a separately configurable option? Just let the user specify the text to display, including an email addresses. Maybe we can support Markdown or something similar.
| type: { | ||
| _type: Type.String, | ||
| _default: 'default', | ||
| _description: 'The type of background to use.', | ||
| _validators: [validators.oneOf(['default', 'color', 'image', 'gradient'])], | ||
| }, | ||
| value: { | ||
| _type: Type.String, | ||
| _default: '', | ||
| _description: | ||
| 'The background value based on the type. ' + | ||
| 'For color: any valid CSS color (e.g., "#0066cc", "rgb(0,102,204)", "blue"). ' + | ||
| 'For gradient: any valid CSS gradient (e.g., "linear-gradient(135deg, #667eea 0%, #764ba2 100%)"). ' + | ||
| 'For image: a URL to the image (e.g., "https://example.com/background.jpg").', | ||
| }, |
There was a problem hiding this comment.
Can we find a way of embedding these into one?
| }, | ||
| alt: { | ||
| _type: Type.String, | ||
| _default: 'Background Image', |
There was a problem hiding this comment.
While this configuration option makes sense, the default value here doesn't.
| size: { | ||
| _type: Type.String, | ||
| _default: 'cover', | ||
| _description: 'Background size for image backgrounds. Options: cover, contain, auto, or custom CSS values.', |
There was a problem hiding this comment.
Ok, by the validator rules out custom CSS values and I'm confused as to what these properties refer to. Can we have either some more expansive text with examples, some simpler options or just forgo this altogether?
(Yes, I know you're referring to a CSS property, but think of who is using this configuration).
- Reduce schema from 386 to 194 lines (50% reduction) - Add _type: Type.Object to all nested objects - Rename columnPosition loginFormPosition - Remove complex CSS styling (card, button sections) - Simplify background to color + imageUrl only - Support translation keys in all text fields - Remove gradient, overlay, contactEmail, customLinks - All 38 tests passing
UjjawalPrabhat
force-pushed
the
feature/configurable-login-page
branch
from
014d395 to
3a74392
Compare
|
@ibacher I’ve now simplified the config schema to exactly what you suggested:
The components and tests (38/38 passing) have been updated to match. Everything builds and works as before—now the schema is clean and focused on simple customization. Ready for final review! |
|
Hey @ibacher, @sherrif10, @tendomart, @denniskigen, just checking in—thanks again for the review. Whenever you have a moment, could you merge this PR? I’m happy to address any last-minute tweaks if needed. Appreciate your help! |
|
Hey @ibacher @sherrif10 @tendomart @denniskigen, any updates on this PR? |
|
Hey @sherrif10 @ibacher @denniskigen @tendomart , Just checking in on this PR: #1460. The implementation now closely matches all requested feedback:
Would appreciate a final review or merge whenever you have time. Let me know if there’s anything else needed! |
6 participants
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.If applicable
Summary
This PR implements a beautiful and configurable login page for OpenMRS 3.x as part of the OMRS Hackathon 2025. The login page now supports:
Related Issue
https://issues.openmrs.org/browse/O3-5027
Other
This work was completed as part of the OpenMRS Hackathon 2025. The implementation focuses on making the login page easily configurable while maintaining a beautiful and professional appearance.