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.

Sign up for GitHub

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

Jump to bottom

(feature) O3-3859 Ability to customize or add logos at the bottom of the LogIn page below the "Powered by" #1127

Merged
merged 6 commits into from
Oct 14, 2024
Merged

(feature) O3-3859 Ability to customize or add logos at the bottom of the LogIn page below the "Powered by" #1127

merged 6 commits into from
Oct 14, 2024

Conversation

suubi-joshua
Copy link
Contributor

@suubi-joshua suubi-joshua commented Aug 27, 2024 •
edited
Loading

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as feat, fix, or chore, among others). See existing PR titles for inspiration.

For changes to apps

If applicable

  • My work includes tests or is validated by existing tests.
  • I have updated the esm-framework mock to reflect any API changes I have made.

Summary

I added a footer config, to allow specification of logos to be included in the footer.

Screenshots

Screencast.from.08-27-2024.09.16.33.PM.webm

image

Related Issue

https://openmrs.atlassian.net/browse/O3-3859
https://openmrs.atlassian.net/browse/O3-3856

Other

@suubi-joshua
Copy link
Contributor Author

suubi-joshua commented Aug 27, 2024 •
edited
Loading

@ibacher @gracepotma @NethmiRodrigo requesting a review here. Not sure why am getting a Network Error.

@ibacher
Copy link
Member

ibacher commented Aug 28, 2024

It shouldn't be possible to remove the OpenMRS logo from the Powered By slot at all.

@suubi-joshua
Copy link
Contributor Author

suubi-joshua commented Aug 28, 2024 •
edited
Loading

It shouldn't be possible to remove the OpenMRS logo from the Powered By slot at all.

Noted @ibacher , I removed the conditional rendering. Now the OpenMRS Logo always renders whether there are more logos specified or not in the configs @gracepotma .

Screencast.from.08-28-2024.10.32.55.PM.webm

@suubi-joshua suubi-joshua mentioned this pull request Sep 3, 2024
Closed
sherrif10
sherrif10 reviewed Sep 10, 2024
View reviewed changes
packages/apps/esm-login-app/src/footer.component.tsx
<svg role="img" className={styles.poweredByLogo}>
<use href="#omrs-logo-full-color"></use>
</svg>
{logos.map((logo, index) => (
Copy link
Member

Choose a reason for hiding this comment

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

Hello @suubi-joshua Basing on @ibacher suggestion, Does this fixes the issue. i thought this can be generic

vasharma05
vasharma05 previously requested changes Sep 19, 2024
View reviewed changes
packages/apps/esm-login-app/__mocks__/config.mock.ts Outdated Show resolved Hide resolved
packages/apps/esm-login-app/__mocks__/config.mock.ts Outdated Show resolved Hide resolved
packages/apps/esm-login-app/src/config-schema.ts Outdated Show resolved Hide resolved
packages/apps/esm-login-app/src/footer.component.tsx Outdated Show resolved Hide resolved
packages/apps/esm-login-app/src/login/login.component.tsx Outdated Show resolved Hide resolved
packages/apps/esm-login-app/src/footer.component.tsx Outdated Show resolved Hide resolved
packages/apps/esm-login-app/src/login/login.scss Outdated Show resolved Hide resolved
packages/apps/esm-login-app/src/login/login.scss
padding: 1.5rem;
}

.logosContainer {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.logosContainer {
.logosContainer::last-of-type {
margin-right: 0;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hard to tell without seeing the dev tools how the margins are working. If you want to clean it up after the merge, @vasharma05 , you are very welcome, I'll give a quick review. But this has been open a long time already, time to get it in.

packages/apps/esm-login-app/src/login/login.scss Outdated Show resolved Hide resolved
packages/apps/esm-login-app/src/login/login.scss Outdated Show resolved Hide resolved
@suubi-joshua
Copy link
Contributor Author

suubi-joshua commented Oct 3, 2024 •
edited
Loading

@gracepotma @ibacher @vasharma05 @brandones @michaelbontyes

@suubi-joshua
Copy link
Contributor Author

New developments here
https://openmrs.slack.com/archives/CKS32D55G/p1727938916121129

brandones
brandones requested changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@brandones brandones left a comment

Choose a reason for hiding this comment

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

Let's get this in and then we can implement the new designs in another PR. I think the config interface should be able to stay the same anyway. Please also address Vineet's feedback.

packages/apps/esm-login-app/src/footer.component.tsx Outdated Show resolved Hide resolved
packages/apps/esm-login-app/src/config-schema.ts Outdated Show resolved Hide resolved
packages/apps/esm-login-app/src/config-schema.ts Outdated Show resolved Hide resolved
packages/apps/esm-login-app/__mocks__/config.mock.ts Outdated Show resolved Hide resolved
@brandones
Copy link
Collaborator

Please fix the CSS so that the styling is unchanged when using defaults. That includes the spacing between "Powered by" and the monochrome OpenMRS logo.

Try and simplify the CSS. I am very mistrustful of the big mess of CSS you have added; I will be surprised if it is not mostly unnecessary.

@suubi-joshua
Copy link
Contributor Author

I have addressed your requests @brandones
Though in my new changes I have made all uploaded logos have a grey to align with the O3 design
Before
image
After
Screenshot from 2024-10-14 14-10-11

brandones
brandones approved these changes Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@brandones brandones 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 @suubi-joshua .

@brandones brandones dismissed vasharma05’s stale review October 14, 2024 13:49

Feedback (mostly) addressed

@brandones brandones merged commit 5e52dc3 into openmrs:main Oct 14, 2024
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandones brandones brandones approved these changes

@vasharma05 vasharma05 Awaiting requested review from vasharma05

@sherrif10 sherrif10 Awaiting requested review from sherrif10

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@suubi-joshua @ibacher @brandones @sherrif10 @vasharma05