-
Notifications
You must be signed in to change notification settings - Fork 19
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
Sandbox css fix on tooltips #1636
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request include the addition of a new CSS class Changes
Possibly related PRs
Suggested reviewers
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: 3
🧹 Outside diff range comments (1)
micro-ui/web/public/index.html (1)
Line range hint
4-11
: Add preconnect hint for performance optimization.Consider adding a preconnect hint for the unpkg.com domain to establish early connection and improve loading performance.
Add this before the stylesheet links:
<meta charset="utf-8" /> + <link rel="preconnect" href="https://unpkg.com"> + <link rel="dns-prefetch" href="https://unpkg.com"> <link rel="icon" href="https://egov-dev-assets.s3.ap-south-1.amazonaws.com/browser-icon.png" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
micro-ui/web/micro-ui-internals/packages/css/package.json
is excluded by!**/*.json
📒 Files selected for processing (2)
- micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (1 hunks)
- micro-ui/web/public/index.html (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (1)
1032-1034
: 🧹 Nitpick (assertive)Consider adding responsive design for mobile devices.
While the fixed width of
30rem
works for desktop, it might cause horizontal scrolling on mobile devices. Consider adding media queries to adjust the tooltip width on smaller screens.Add responsive design:
.tooltiptext{ width: 30rem !important; + @media (max-width: 768px) { + width: 90vw !important; + } }Let's verify the tooltip's appearance on mobile screens:
@@ -7,7 +7,7 @@ | |||
<link | |||
href="https://fonts.googleapis.com/css2?family=Roboto+Condensed:wght@400;500;700&family=Roboto:wght@400;500;700&display=swap" | |||
rel="stylesheet" type="text/css" /> | |||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected].47/dist/index.css" /> | |||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected].48/dist/index.css" /> |
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.
🧹 Nitpick (assertive)
Consider adding a local fallback for CDN resources.
To ensure the application remains functional even if the CDN is unavailable, consider implementing a fallback mechanism.
Example implementation:
// Add this script after your stylesheet link
<script>
function fallbackToLocalCSS(e) {
const link = e.target;
const localLink = document.createElement('link');
localLink.rel = 'stylesheet';
localLink.href = '/assets/css/digit-ui.css'; // Local fallback path
document.head.appendChild(localLink);
}
document.querySelector('link[href*="digit-ui-css"]').addEventListener('error', fallbackToLocalCSS);
</script>
Add integrity hash for enhanced security.
The CDN stylesheet link should include an integrity hash to prevent malicious code injection through CDN compromises.
Apply this change:
- <link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" />
+ <link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css"
+ integrity_no="sha384-[HASH]"
+ crossorigin="anonymous" />
Note: Replace [HASH]
with the actual SHA-384 hash of the CSS file.
Committable suggestion was skipped due to low confidence.
💡 Codebase verification
Based on the search results, I can now provide a definitive response about the tooltip width change impact. The codebase shows that:
- The tooltip width is indeed set to
30rem !important
insandbox.scss
- The base tooltip styles in
tooltip.scss
show that tooltips are positioned absolutely and have responsive behavior with opacity transitions - No specific media queries were found for tooltip width adjustments on smaller screens
The 30rem width (approximately 480px) could cause layout issues on mobile devices with smaller viewport widths.
Here are the specific concerns:
- Mobile devices typically have viewport widths between 320px-480px
- The fixed 30rem width with
!important
will override any responsive adjustments - The tooltip could overflow the screen on smaller devices
🔗 Analysis chain
Verify the tooltip width change impact.
The version update includes a .tooltiptext
class with width: 30rem !important
. Please ensure this change doesn't cause layout issues on smaller screens.
Let me try a different approach to search for tooltip-related code and styles.
This should help us find any tooltip-related code regardless of the file extension and also list any CSS files that might contain tooltip styles.
Based on the search results, I can see that the tooltip styles are defined in multiple files, with the main tooltip styles in micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/tooltip.scss
and micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss
. Let me check the specific tooltip width implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tooltip-related styles and usage
echo "Searching for tooltip-related styles..."
rg -t scss "tooltip"
echo "Searching for tooltip-related components..."
rg -t jsx -t tsx "tooltip"
Length of output: 290
Script:
#!/bin/bash
# Search for tooltip-related styles and usage without file type restrictions
echo "Searching for tooltip-related styles and components..."
rg "tooltip" -i -A 2 -B 2
# Also check if there are any CSS files directly
echo -e "\nListing CSS files in the repository..."
fd ".css$"
Length of output: 79279
Script:
#!/bin/bash
# Check the content of the tooltip style files
echo "Content of tooltip.scss:"
cat micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/tooltip.scss
echo -e "\nContent of sandbox.scss:"
cat micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss
# Also check for any media queries related to tooltips
echo -e "\nChecking for tooltip-related media queries:"
rg "tooltip.*@media|@media.*tooltip" -A 3
Length of output: 20082
Summary by CodeRabbit
New Features
.tooltiptext
to enforce tooltip width across the application.Chores
index.html
to the latest version of@egovernments/digit-ui-css
.