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

[B5] Web Apps #34682

Open
wants to merge 82 commits into
base: master
Choose a base branch
from
Open

[B5] Web Apps #34682

wants to merge 82 commits into from

Conversation

orangejenny
Copy link
Contributor

@orangejenny orangejenny commented May 28, 2024

Product Description

Turn on bootstrap 5 for web apps. Side by side comparisons are here, but the high level summary is: not much changed. Colors are a little brighter, borders are a little lighter, buttons are a little bigger.

Technical Summary

Review by commit. Most commits are small, except the cleanup commits, which are mechanical. Most of this work has been merged already, what remains in this PR is three major changes:

  • Updating the python views to use the B5 decorators and templates
  • Javascript changes that either depend on B5 libraries (bootstrap js or tempus dominus) or were annoying to wrap behind a window.USE_BOOTSTRAP5 check.
  • Deleting the B3 templates and diff files.

Safety Assurance

Safety story

High risk, as this is a high visibility area, there are a lot of changes (including changes made in previous PRs that will go live when this is merged), and our test coverage at the UI level is limited. We'll be going through the same QA/safety approach as for the web apps requirejs migration.

Automated test coverage

Not a whole lot.

QA Plan

https://dimagi.atlassian.net/browse/QA-6550

Rollback instructions

This can be rolled back if needed. Due to its size, it may get difficult to automatically roll back relatively soon, although it looks like there hasn't been that much activity in the web apps front end in the past few weeks.

Sentry can be monitored for javascript errors using this filter.

  • 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

@orangejenny orangejenny added awaiting QA QA in progress. Do not merge product/all-users-all-environments Change impacts all users on all environments Risk: High Change affects files that have been flagged as high risk. labels May 28, 2024
@orangejenny orangejenny requested review from a team May 28, 2024 18:03
@orangejenny orangejenny removed the request for review from a team May 28, 2024 18:03
@orangejenny orangejenny requested a review from nospame as a code owner May 28, 2024 18:03
@dimagimon dimagimon added dependencies Pull requests that update a dependency file Risk: Medium Change affects files that have been flagged as medium risk. and removed Risk: High Change affects files that have been flagged as high risk. labels May 28, 2024
corehq/apps/cloudcare/static/cloudcare/js/utils.js Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
---
+++
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these not unsplit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're still used here:

<link type="text/less"
rel="stylesheet"
media="all"
href="{% static 'cloudcare/less/font-formplayer.less' %}" />
<link type="text/less"
rel="stylesheet"
media="all"
href="{% static 'cloudcare/less/formplayer-common.less' %}"/>
<link type="text/less"
rel="stylesheet"
media="all"
href="{% static 'cloudcare/less/formplayer-webapp.less' %}"/>

This is basically the result of running the bootstrap migration tool.
- dateTimePickerTooltips are already handled by by defaultTranslations in the HQ tempus dominus module
- Several callbacks/handlers were no longer needed
- It's annoying that showing the clear button is handled through initial page data, but updating that is out of scope
The migration tool doesn't check js files for CSS class names,
and some of these aren't greppable anyway.
This completes changes that were started in 845d9ff
orangejenny and others added 21 commits June 10, 2024 14:26
This behavior is not added to the time picker or the date range picker since
neither of those are single-click interactions.
This makes tempus dominus more forgiving about users entering single-digit
months and years, which would previously be rejected as invalid input.
Copy link
Contributor

@MartinRiese MartinRiese left a comment

Choose a reason for hiding this comment

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

started backwards because I am not sure how far I got before. Stopped at 15e030b

Copy link
Contributor

@MartinRiese MartinRiese left a comment

Choose a reason for hiding this comment

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

got to 5858f7c

@@ -0,0 +1,7 @@
.query-caption.required label::before, .search-query-group-header.required .search-query-group-header-label::before {
content: '*';
Copy link
Contributor

Choose a reason for hiding this comment

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

🤯 you can set content in css?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we live in a magical world

@@ -34,10 +34,3 @@ legend {
border-top: $cc-neutral-hi solid 1px;
padding-top: 20px;
}

.question-tile-row {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@MartinRiese MartinRiese left a comment

Choose a reason for hiding this comment

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

new changes look good

* Bootstrap 5 removes the top margin of h1-h6. This affects the
  collapsible group header layout
* Add a class to add an equal margin for h1-h6 top and bottom
* Use it for group headers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting QA QA in progress. Do not merge dependencies Pull requests that update a dependency file product/all-users-all-environments Change impacts all users on all environments 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.

None yet

3 participants