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 formatting on No results found modal #8346

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DavidMcClatchey
Copy link
Collaborator

FRONTEND PULL REQUEST

Related Issue

Changes Proposed

  • This resolves a display error when no results are returned in the "Search by name" field of the Results -> View test results page. It now looks the same as the display on the "Conduct Tests" page search field when no results are returned.

Testing

  • Tested locally. I didn't deploy to a dev env since it seems simple enough, but if there's reason to deploy to a dev env please let me know.

Screenshots / Demos

Screenshot 2024-12-06 at 3 43 07 PM

bobbywells52
bobbywells52 previously approved these changes Dec 10, 2024
@mehansen
Copy link
Collaborator

do we want this modal to look the same as the one on the Conduct tests page when viewed on mobile?

mpbrown
mpbrown previously approved these changes Dec 11, 2024
Copy link
Collaborator

@mpbrown mpbrown left a comment

Choose a reason for hiding this comment

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

LGTM locally!

Copy link
Collaborator

@mehansen mehansen left a comment

Choose a reason for hiding this comment

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

I wonder if we should replicate some of what's going on here
https://github.com/CDCgov/prime-simplereport/blob/main/frontend/src/scss/base/_prime-styles.scss#L381-L397 with the way the width adapts to screen size

@DavidMcClatchey
Copy link
Collaborator Author

DavidMcClatchey commented Dec 18, 2024

I wonder if we should replicate some of what's going on here https://github.com/CDCgov/prime-simplereport/blob/main/frontend/src/scss/base/_prime-styles.scss#L381-L397 with the way the width adapts to screen size

@mehansen it looks like the formatting in_prime-styles is already included in the modal style apart from what gets overridden by the "higher specificity" style that's modified in the PR.

Screenshot 2024-12-18 at 10 40 15 AM

DanielSass
DanielSass previously approved these changes Dec 18, 2024
@mehansen
Copy link
Collaborator

mehansen commented Dec 18, 2024

@mehansen it looks like the formatting in_prime-styles is already included in the modal style apart from what gets overridden by the "higher specificity" style that's modified in the PR.

@DavidMcClatchey sorry I meant to point out that we do have this responsive width behavior that's already defined for modals on both pages. so I was wondering why make them different by setting max-width in the first place? it essentially hides the behavior and leads to the modals not looking the same across certain page widths

for the record this is not blocking since the PR solves the issue

@DavidMcClatchey
Copy link
Collaborator Author

@mehansen it looks like the formatting in_prime-styles is already included in the modal style apart from what gets overridden by the "higher specificity" style that's modified in the PR.

@DavidMcClatchey sorry I meant to point out that we do have this responsive width behavior that's already defined for modals on both pages. so I was wondering why make them different by setting max-width in the first place? it essentially hides the behavior and leads to the modals not looking the same across certain page widths

for the record this is not blocking since the PR solves the issue

Good call, it looks like removing the max width setting also had the effect of fixing the formatting issue, so I've pushed that up as a cleaner solution. I also saw the position: absolute; was redundant with what's already in _prime-styles so I've removed that as well.

@mehansen mehansen self-requested a review December 23, 2024 23:03
Copy link
Collaborator

@mehansen mehansen left a comment

Choose a reason for hiding this comment

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

thank you!

Copy link
Collaborator

@mpbrown mpbrown 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 for the cleaner fix!

Copy link
Collaborator

@bobbywells52 bobbywells52 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 for the clean fix!

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.

[Bug] Fix formatting on No results found modal
5 participants