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) Enhancement on navigation button style #132

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

icrc-jofrancisco
Copy link
Contributor

@icrc-jofrancisco icrc-jofrancisco commented Mar 27, 2024

Requirements

  • This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

In this pull request, I've made modifications to the navigation buttons within the form. Currently, the buttons have a "header-like" styling, leading to an oversized font. To enhance consistency across sections and text elements, I've realigned the button style to match that of other components.

Screenshots

Before:
Pasted Graphic 1

After:
Pasted Graphic

Related Issue

Other

Thanks,

@denniskigen
Copy link
Member

Thanks for the PR, @icrc-jofrancisco. Might you have read this thread https://openmrs.slack.com/archives/CKS32D55G/p1710226081710819? Just adding it before I get to reviewing it

@icrc-jofrancisco
Copy link
Contributor Author

Thanks for the PR, @icrc-jofrancisco. Might you have read this thread https://openmrs.slack.com/archives/CKS32D55G/p1710226081710819? Just adding it before I get to reviewing it

Thanks for your comment @denniskigen .
I read the comments carefully and I think the outcome of my implementation is aligned with the expected result, specifically the resizing of the font. There was also a suggestion to change the color of the previous/next labels (also done in this PR).
That said, I think the PR is ready for review.
Thank you

Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

Thanks, @icrc-jofrancisco. I've left a suggestion. Let's see how this looks and evaluate from there.

@@ -23,15 +23,15 @@

.nav-link-text {
margin-top: 0.125rem;
white-space: nowrap;
Copy link
Member

@denniskigen denniskigen Mar 28, 2024

Choose a reason for hiding this comment

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

I think it's important to keep this around for visual consistency and balance. Having the page name truncated when the titles are too long isn't necessarily a bad trade-off when the full page names are listed on the left panel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Thanks.

@denniskigen denniskigen changed the title (style) Enhancement on navigation button style (feat) Enhancement on navigation button style Mar 28, 2024
}

.nav-label {
@include type.type-style('label-01');
color: #696969;
Copy link
Member

@denniskigen denniskigen Mar 28, 2024

Choose a reason for hiding this comment

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

#525252 is the default carbon hex for input labels. It's probably a better fit here for consistency:

CleanShot 2024-03-28 at 1  03 43@2x

Also, when we migrate the form engine to use carbon colour tokens, we could swap it out for gray-70.

It also gets a better contrast score than #696969:

CleanShot 2024-03-28 at 1  06 37@2x

CleanShot 2024-03-28 at 1  08 04@2x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Thanks. @denniskigen

@denniskigen
Copy link
Member

Thanks, @icrc-jofrancisco!

@denniskigen denniskigen merged commit e147e02 into openmrs:main Mar 28, 2024
3 checks passed
@denniskigen
Copy link
Member

Follow-up to say this looks great to me in prod. Thanks for working on it, @icrc-jofrancisco!

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.

2 participants