-
-
Notifications
You must be signed in to change notification settings - Fork 218
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
Redesign enterprise tile #35622
base: master
Are you sure you want to change the base?
Redesign enterprise tile #35622
Conversation
per Sud's suggestion
Sud: the text size is big enough to negate the need for making it bold
{% if report.total_description %} | ||
<div class="position-absolute start-100" style="top: 25%"> | ||
<span class="hq-help-template" data-content="{{ report.total_description }}" data-placement="right"> | ||
<i class="fa fa-info-circle"></i> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you think of putting the icon next to the header text instead of the number? It's more secondary/tertiary information than primary information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel putting the icon next to the header text implies this description is to explain the header text, not the stat there anymore. But maybe I can move the icon to next to the header text, and then in the tooltip, I included a description of the stat, also a description of the report they will get. I'll bring it up with saas-product. Thank you!
<i class="fa fa-spin fa-spinner"></i> | ||
</div> | ||
{% if report.total_description %} | ||
<div class="position-absolute start-100" style="top: 25%"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The top: 25%
style could be a top-25
class, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bootstrap only have top-0
, top-50
and top-100
, so I have to use style: https://getbootstrap.com/docs/5.0/utilities/position/#arrange-elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha, got it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can override the sass map to include top-25
https://getbootstrap.com/docs/5.3/utilities/position/#sass-maps
also i would prefer end-0
instead of start-100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
override in 9119c9f
start-100
put the left edge of the element at the right edge of the parent element
end-0
put the right edge of the element at the left edge of the parent element
So they're not exchangeable...
corehq/apps/enterprise/templates/enterprise/partials/project_tile.html
Outdated
Show resolved
Hide resolved
Fix a small thing as well: <i> element will be override by "hq-help-template" anyway so remove it
WalkthroughThe pull request introduces a series of changes across multiple files in the CommCare HQ enterprise application, primarily focusing on transitioning from API usage tracking to API key management. The modifications span the API resources, enterprise reporting system, and associated templates and styling. Key changes include the removal of the These changes represent a systematic update to the enterprise reporting and API management components of the application, focusing on nomenclature and structural improvements. Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
corehq/apps/enterprise/static/enterprise/js/project_dashboard.js (1)
228-228
: Consider consolidating tooltip visibility management.The visibility of the help tooltip is managed in multiple places. Consider extracting this logic into a dedicated function to improve maintainability and reduce the risk of inconsistent states.
function updateDisplayTotal($element, kwargs) { const $display = $element.find(".js-total"); const $helpTotal = $element.find(".help-total"); + + function setHelpTotalVisibility(visible) { + $helpTotal[visible ? 'removeClass' : 'addClass']('d-none'); + } + const slug = $element.data("slug"); + setHelpTotalVisibility(false); // Hide initially const requestParams = { url: initialPageData.reverse("enterprise_dashboard_total", slug), success: function (data) { $display.html(localizeNumberlikeString(data.total)); - $helpTotal.removeClass("d-none"); + setHelpTotalVisibility(true); }, error: function (request) { if (request.responseJSON) { alertUser.alert_user(request.responseJSON["message"], "danger"); } else { alertUser.alert_user(gettext("Error updating display total, please try again or report an issue if this persists."), "danger"); } $display.html(gettext("??")); - $helpTotal.addClass("d-none"); + setHelpTotalVisibility(false); }, data: kwargs, }; $display.html('<i class="fa fa-spin fa-spinner"></i>'); - $helpTotal.addClass("d-none"); $.ajax(requestParams); }Also applies to: 234-234, 243-243, 248-248
corehq/apps/enterprise/templates/enterprise/partials/project_tile.html (1)
34-43
: Remove unnecessary non-breaking space.The
is used as a placeholder but could be removed since the button already has content from the data binding.<button class="btn btn-link fs-6" id="{{ report.slug }}_dateRangeDisplay" type="button" data-bind="text: presetText" data-bs-toggle="modal" data-bs-target="#enterpriseFormsDaterange" data-sender="{{ report.slug }}" - > - - </button> + ></button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
corehq/apps/enterprise/static/enterprise/js/project_dashboard.js
(2 hunks)corehq/apps/enterprise/templates/enterprise/partials/project_tile.html
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: task-list-completed
- GitHub Check: tests (python-sharded-and-javascript)
- GitHub Check: tests (python, bf)
- GitHub Check: tests (python, 6a)
- GitHub Check: tests (python, 05)
- GitHub Check: task-list-completed
🔇 Additional comments (2)
corehq/apps/enterprise/templates/enterprise/partials/project_tile.html (2)
Line range hint
49-53
: Button style update looks good.The change from primary to outline style aligns with the PR objectives and style guide requirements.
12-27
: Consider a more responsive approach for tooltip positioning.The absolute positioning with
start-100 top-25
might cause layout issues on different screen sizes. Consider using a more flexible positioning approach.
Product Description
Changes:
Before:
After:
Technical Summary
Ticket: https://dimagi.atlassian.net/browse/SAAS-16428
Feature Flag
Safety Assurance
Safety story
Mainly appearance change.
Automated test coverage
QA Plan
Rollback instructions
Labels & Review