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

Sticky case tile in web apps #33819

Merged
merged 29 commits into from
Jan 16, 2024
Merged

Sticky case tile in web apps #33819

merged 29 commits into from
Jan 16, 2024

Conversation

orangejenny
Copy link
Contributor

@orangejenny orangejenny commented Nov 30, 2023

Product Description

https://dimagi-dev.atlassian.net/browse/USH-3980

Make persistent case tiles stick to the top of the screen. This will affect all persistent case tiles, whether they're in a case list or a form.

Also makes a few minor styling adjustments. Review by commit.

Screen Shot 2023-11-29 at 8 23 37 PM Screen Shot 2023-11-29 at 8 23 47 PM

Feature Flag

Case tiles

Safety Assurance

Safety story

This is all styling changes, but I'm requesting QA to make sure none of the "cleanup" has unwanted side effects.

Automated test coverage

doubt it

QA Plan

https://dimagi-dev.atlassian.net/browse/QA-5877

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

…t-container

This is a semantic change: content belongs in the content container, not after it.
Now that #webforms is inside .content-container, which is also a .container,
it's getting double padding. Fix that.
There are enough outer containers.
Unnecessary and causes inconsistency with other screens
This may be why #webforms was outside of .form-container: on app
preview, there's no .form-container. Deal with this with a style
in an app preview file.
This is another side effect of removing the double .container
Also nest some related styles together.
@orangejenny orangejenny added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Nov 30, 2023
@dimagimon dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label Nov 30, 2023
@orangejenny orangejenny added the awaiting QA QA in progress. Do not merge label Nov 30, 2023
@@ -97,6 +97,7 @@
@checkbox-default-color: @call-to-action-mid;

@breadcrumb-separator: '>';
@breadcrumb-height-cloudcare: 46px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@biyeun I don't need to do anything in B5, right, since the Web Apps migration hasn't started?

Copy link
Member

@biyeun biyeun Jan 11, 2024

Choose a reason for hiding this comment

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

yes, correct. you can add that in when you are migrating web apps

Copy link
Contributor

@nospame nospame left a comment

Choose a reason for hiding this comment

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

This will affect all persistent case tiles, whether they're in a case list or a form

Does this interact at all with the sticky map (any screen size) or map+header (on small screens)?

@@ -134,6 +134,18 @@

}

.case-tile-container {
position: sticky;
position: -webkit-sticky;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still necessary? It looks like Safari supports regular position: sticky since late 2019.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks - a457a02

@orangejenny
Copy link
Contributor Author

Requesting review of recent commits.

Fixed https://dimagi-dev.atlassian.net/browse/QA-5983 with e9d5bc9, which removed a 10+-year-old line that automatically adds ui-widget and ui-widget-container classes to all container classes. ui-widget and ui-widget-container are jQuery UI classes, so I figure that dates back to a time when jQuery UI was used much more extensively. I've convinced myself this isn't that big a deal grepping the codebase for container and for ui-widget. Hardly any pages even use container outside of web apps, and they're mostly simple pages that don't use jQuery UI widgets (domain_request.html, activate_transfer_domain.html, etc.).

FYI @biyeun

@biyeun
Copy link
Member

biyeun commented Jan 15, 2024

@orangejenny noting that we chatted about this offline and that my recommendation is to put this change on staging and see if it causes any issues. if not it's g2g from my perspective

@orangejenny
Copy link
Contributor Author

@biyeun yep, it's been on staging for a few days without issue and I've done some light smoke testing on pages using jQuery UI

Copy link
Contributor

@stephherbers stephherbers 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 for the detailed commit messages

@orangejenny orangejenny added QA Passed and removed awaiting QA QA in progress. Do not merge labels Jan 16, 2024
@orangejenny orangejenny merged commit 6189db0 into master Jan 16, 2024
11 of 12 checks passed
@orangejenny orangejenny deleted the jls/web-apps-sticky-tile branch January 16, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled QA Passed Risk: Medium Change affects files that have been flagged as medium risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants