refactor(web): plugin playground presets - refactor presets styling [VIZ-1384] #1495
Conversation
WalkthroughThis change modifies the styling approach of the responsivePanel, header, and sidebar widgets within PluginPlayground. It removes the import of a common preset style and introduces external CSS file imports directly into the widgets’ sourceCode. The overall structure of the widgets and their YAML configurations remain unchanged. Changes
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
OpeDada
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/beta/features/PluginPlayground/Plugins/presets/ui/responsivePanel.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci-web / ci
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
web/src/beta/features/PluginPlayground/Plugins/presets/ui/responsivePanel.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/PluginPlayground/Plugins/presets/ui/responsivePanel.ts
Show resolved
Hide resolved
web/src/beta/features/PluginPlayground/Plugins/presets/ui/responsivePanel.ts
Outdated
Show resolved
Hide resolved
…m:reearth/reearth into refactor/plugin-playground-common-styling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/src/beta/features/PluginPlayground/Plugins/presets/ui/sidebar.ts (1)
34-34: Consider hosting the CSS file locally to reduce external dependencies.While importing a common CSS file promotes consistent styling across plugins, relying on an external URL introduces a dependency that could affect reliability if the resource becomes unavailable. Consider hosting this file locally within your project, especially if it's critical for functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/src/beta/features/PluginPlayground/Plugins/presets/ui/header.ts(1 hunks)web/src/beta/features/PluginPlayground/Plugins/presets/ui/responsivePanel.ts(2 hunks)web/src/beta/features/PluginPlayground/Plugins/presets/ui/sidebar.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/features/PluginPlayground/Plugins/presets/ui/responsivePanel.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
- GitHub Check: ci-web / ci
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
🔇 Additional comments (4)
web/src/beta/features/PluginPlayground/Plugins/presets/ui/sidebar.ts (2)
91-91: Good addition of the "input" class to the search bar.Adding the "input" class to the search bar element helps maintain consistent styling with other input elements that may be using the same class from the imported CSS framework. This improves UI consistency.
28-28: Well-documented component purpose.Adding a descriptive comment about the sidebar's purpose is a good practice that improves code readability and helps other developers understand the component's functionality at a glance.
web/src/beta/features/PluginPlayground/Plugins/presets/ui/header.ts (2)
41-41: Verify the visual impact of removing vertical padding.The padding has been changed from
10px 16pxto0 16px, removing the vertical padding completely. Please verify that this doesn't make the header appear too compact or cause elements to touch the top/bottom edges of the header container.
54-59: Good refactoring of menu item styles.The simplified CSS for menu items removes specific styling properties (font-size, color, cursor, etc.) while adding basic reset properties (background: none, border: none). This suggests these properties are now handled by the imported CSS file, which promotes consistency across the UI.
|
PR looks better and better. Would need to update the PR with the latest main branch. |
web/src/beta/features/PluginPlayground/Plugins/presets/ui/header.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/PluginPlayground/Plugins/presets/ui/header.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/PluginPlayground/Plugins/presets/ui/header.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/PluginPlayground/Plugins/presets/ui/header.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/PluginPlayground/Plugins/presets/ui/sidebar.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/PluginPlayground/Plugins/presets/ui/sidebar.ts
Outdated
Show resolved
Hide resolved
web/src/beta/features/PluginPlayground/Plugins/presets/ui/sidebar.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web/src/beta/features/PluginPlayground/Plugins/presets/ui/header.ts (1)
55-61: Consider adding proper semantic attributes.For better accessibility, consider adding proper semantic attributes to the menu items such as aria-labels or roles. Also, these list items would benefit from having actual links if they're meant to be interactive.
- <li>Home</li> - <li>About</li> - <li>Services</li> - <li>Contact</li> - <li>FAQ</li> + <li><a href="#home" role="menuitem">Home</a></li> + <li><a href="#about" role="menuitem">About</a></li> + <li><a href="#services" role="menuitem">Services</a></li> + <li><a href="#contact" role="menuitem">Contact</a></li> + <li><a href="#faq" role="menuitem">FAQ</a></li>web/src/beta/features/PluginPlayground/Plugins/presets/ui/sidebar.ts (3)
38-43: Fixed height may limit responsiveness.Using a fixed
max-height: 580pxcould create issues on different screen sizes. Consider using a relative unit or viewport-based approach for better responsiveness.- max-height: 580px; + max-height: 80vh; /* or use a CSS variable for consistent sizing */
146-161: Consider refactoring the toggle logic.The current toggle logic uses direct style manipulation which is less maintainable. Consider using class toggling for all style changes instead of mixing approaches.
toggleBtn.addEventListener("click", () => { const sidebar = document.querySelector("#wrapper"); const isCollapsed = sidebar.style.width === "60px"; - // Collapse or expand the sidebar - sidebar.style.width = isCollapsed ? "250px" : "60px"; + // Toggle collapsed state through class + sidebar.classList.toggle("sidebar-collapsed", !isCollapsed); // Toggle visibility instead of display for title sidebarTitle.classList.toggle("invisible", !isCollapsed); // Hide or show menu titles const titles = document.querySelectorAll(".menu-title"); titles.forEach((title) => { - title.style.display = isCollapsed ? "inline" : "none"; + title.classList.toggle("invisible", !isCollapsed); }); });Then add these styles:
.sidebar-collapsed { width: 60px; } #wrapper { width: 250px; transition: width 0.3s ease; }
88-137: Consider using proper semantic elements for menu items.For better accessibility and user experience, consider making the menu items proper interactive elements (like buttons or links) instead of just spans with icons and text.
- <li> - <span class="menu-icon">🏠</span> - <span class="menu-title">Home</span> - <span class="menu-dots">⋮</span> - </li> + <li> + <a href="#home" class="menu-item"> + <span class="menu-icon">🏠</span> + <span class="menu-title">Home</span> + <span class="menu-dots">⋮</span> + </a> + </li>And add this CSS:
.menu-item { display: flex; align-items: center; width: 100%; text-decoration: none; color: inherit; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/beta/features/PluginPlayground/Plugins/presets/ui/header.ts(1 hunks)web/src/beta/features/PluginPlayground/Plugins/presets/ui/sidebar.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
- GitHub Check: ci-server / ci-server-test
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-web / ci
🔇 Additional comments (8)
web/src/beta/features/PluginPlayground/Plugins/presets/ui/header.ts (4)
28-29: Good documentation improvement.Adding a descriptive comment that explains the component's purpose helps other developers understand the code more easily.
31-34: Appropriate styling architecture change.Moving from a direct import of common styles to an external CSS import via URL is consistent with the refactoring goals. The added comment clarifies the purpose of this import system.
44-48: Simplified menu styling.The styles for .header-menu have been significantly simplified, removing properties like font-size, color, cursor, etc. This approach is better as these styles are now likely provided by the imported preset-ui.css.
36-42:❓ Verification inconclusive
Verify padding change impact.
The padding was changed from what appears to have been "10px 16px" to "0 16px", removing the vertical padding. Please ensure this doesn't negatively affect the vertical spacing/alignment of the header content.
🏁 Script executed:
#!/bin/bash # Check if there are other components or tests that might be affected by this padding change rg -g "*.{ts,tsx,js,jsx}" "header.*padding" --no-filenameLength of output: 59
Action Required: Manually Verify Header Vertical Spacing
The padding in
web/src/beta/features/PluginPlayground/Plugins/presets/ui/header.ts(lines 36–42) has been changed from an assumed “10px 16px” to “0 16px”, effectively removing vertical padding. An automated search for additional references to header padding did not reveal any conflicting settings. However, please perform a thorough manual visual review to ensure that this reduction in vertical padding does not adversely affect the alignment or spacing of header content in the UI.web/src/beta/features/PluginPlayground/Plugins/presets/ui/sidebar.ts (4)
28-30: Good documentation improvement.Adding a descriptive comment that explains the component's purpose helps other developers understand the code more easily.
31-34: Appropriate styling architecture change.Moving from a direct import of common styles to an external CSS import via URL is consistent with the refactoring goals. The added comment clarifies the purpose of this import system.
84-84: Good improvement with input class.Adding the "input" class to the search bar improves consistency with the preset styling system and likely applies standard input styling from the imported CSS.
154-154: Addressed previous review comment.Changed from "hidden" to "invisible" class as suggested in a previous review. This is consistent with the preset styling system's naming conventions.
….com:reearth/reearth into refactor/plugin-playground-common-styling
Overview
Refactored the preset plugins based on a style guide currently in this file:
web/src/beta/features/PluginPlayground/Plugins/presets/common.tsTried to get all the plugins to use this style guide, removed CSS from the preset-plugins and made sure the UI is consistent across all plugins.
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Summary by CodeRabbit