Conversation
Added CSS asset registration for dark mode support.
Contributor
Reviewer's GuideAdds a dedicated stylesheet and semantic CSS class for the GitHub portfolio module to support dark mode and centralize styling, while wiring the new asset into the module manifest and template. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
.github-portfolio-cardclass used in the JS doesn’t have any corresponding rules ingithub-portfolio.css(the CSS targets.github-portfolio-module .pr-cardinstead), so the card styling and intended dark-mode support likely won’t apply as expected; align the class names between JS and CSS. - By removing the inline
backgroundColorandborderon the card without reintroducing equivalent styles in the new stylesheet (including a dark-mode variant), the visual appearance may regress in light mode; consider moving those styles intogithub-portfolio.cssalongside any dark-mode overrides.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `.github-portfolio-card` class used in the JS doesn’t have any corresponding rules in `github-portfolio.css` (the CSS targets `.github-portfolio-module .pr-card` instead), so the card styling and intended dark-mode support likely won’t apply as expected; align the class names between JS and CSS.
- By removing the inline `backgroundColor` and `border` on the card without reintroducing equivalent styles in the new stylesheet (including a dark-mode variant), the visual appearance may regress in light mode; consider moving those styles into `github-portfolio.css` alongside any dark-mode overrides.
## Individual Comments
### Comment 1
<location path="src/modules/mod_github_portfolio/media/js/github-portfolio.js" line_range="95" />
<code_context>
- card.className = 'p-4 rounded shadow h-100';
- card.style.backgroundColor = '#f8f9fa';
- card.style.border = '1px solid #dee2e6';
+ card.className = 'p-4 rounded shadow h-100 github-portfolio-card';
const h4 = document.createElement('h4');
</code_context>
<issue_to_address>
**issue (bug_risk):** Align the new `github-portfolio-card` class with the CSS selectors to avoid a visual regression.
Currently the CSS only styles `.github-portfolio-module .pr-card`, so `.github-portfolio-card` won’t receive the background/border styles, causing a visual regression. Either update the JS to use `pr-card` or extend the CSS to also target `.github-portfolio-card` so the styles apply.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Added support for .github-portfolio-card and dark mode styles.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new CSS comment
/* Supporta sia .pr-card (retrocompatibilità) che .github-portfolio-card */is in Italian while the rest of the module appears to use English, which may be confusing for maintainers; consider rewriting it in English for consistency. - In mod_github_portfolio.xml, the new
<folder>css</folder>line uses a tab-based indentation that differs from the surrounding space-based indentation; align the indentation style with the existing XML for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new CSS comment `/* Supporta sia .pr-card (retrocompatibilità) che .github-portfolio-card */` is in Italian while the rest of the module appears to use English, which may be confusing for maintainers; consider rewriting it in English for consistency.
- In mod_github_portfolio.xml, the new `<folder>css</folder>` line uses a tab-based indentation that differs from the surrounding space-based indentation; align the indentation style with the existing XML for consistency.
## Individual Comments
### Comment 1
<location path="src/modules/mod_github_portfolio/media/css/github-portfolio.css" line_range="40-4" />
<code_context>
+}
+
+/* Responsive adjustments */
+@media (max-width: 768px) {
+ .github-portfolio-module .container {
+ padding-left: 15px;
+ padding-right: 15px;
+ }
+
+ .github-portfolio-module .pr-card,
+ .github-portfolio-module .github-portfolio-card {
+ margin-bottom: 1.5rem;
</code_context>
<issue_to_address>
**suggestion:** Narrow the `.container` selector to avoid unintentionally affecting nested containers.
This selector changes padding for every `.container` within `.github-portfolio-module`, including any nested containers from other components. To avoid unintended layout changes, target only the intended wrapper (e.g., by giving it a dedicated class) instead of the generic `.container`.
Suggested implementation:
```
/* Responsive adjustments */
@media (max-width: 768px) {
/* Use a dedicated container class to avoid affecting nested .container elements */
.github-portfolio-module .github-portfolio-container {
padding-left: 15px;
padding-right: 15px;
}
.github-portfolio-module .pr-card,
.github-portfolio-module .github-portfolio-card {
margin-bottom: 1.5rem;
}
}
```
To fully implement this change, update the module’s markup so that the intended layout wrapper inside `.github-portfolio-module` uses the new class. For example:
- Replace `class="container"` with `class="container github-portfolio-container"` (or add `github-portfolio-container` alongside any existing classes) on the specific wrapper element whose padding should change at this breakpoint.
This ensures only that wrapper gets the responsive padding while other nested `.container` elements remain unaffected.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /* GitHub Portfolio Module Styles */ | ||
|
|
||
| /* Supporta sia .pr-card (retrocompatibilità) che .github-portfolio-card */ | ||
| .github-portfolio-module .pr-card, |
Contributor
There was a problem hiding this comment.
suggestion: Narrow the .container selector to avoid unintentionally affecting nested containers.
This selector changes padding for every .container within .github-portfolio-module, including any nested containers from other components. To avoid unintended layout changes, target only the intended wrapper (e.g., by giving it a dedicated class) instead of the generic .container.
Suggested implementation:
/* Responsive adjustments */
@media (max-width: 768px) {
/* Use a dedicated container class to avoid affecting nested .container elements */
.github-portfolio-module .github-portfolio-container {
padding-left: 15px;
padding-right: 15px;
}
.github-portfolio-module .pr-card,
.github-portfolio-module .github-portfolio-card {
margin-bottom: 1.5rem;
}
}
To fully implement this change, update the module’s markup so that the intended layout wrapper inside .github-portfolio-module uses the new class. For example:
- Replace
class="container"withclass="container github-portfolio-container"(or addgithub-portfolio-containeralongside any existing classes) on the specific wrapper element whose padding should change at this breakpoint.
This ensures only that wrapper gets the responsive padding while other nested.containerelements remain unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a dedicated stylesheet and CSS classes for the GitHub portfolio module to support dark mode and responsive styling, and wire it into the module assets.
Enhancements:
Build: