Add Research Subdirectory #1
Merged
Conversation
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
Reviewer's GuideThis pull request introduces a new “Research” section—complete with dedicated CSS, Eleventy layouts, and content—while cleaning up and standardizing existing CSS and JS, adjusting templates, and updating navigation and site configuration. 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}
There was a problem hiding this comment.
Hey @Apoo711 - I've reviewed your changes - here's some feedback:
- I noticed you’ve manually added the Research nav link in many HTML templates—extract that into a shared include or macro to keep it DRY and prevent drift when you need to update it in the future.
- The new research.css is quite large; consider splitting it into smaller component or utility files (or reusing existing base/component styles) to reduce duplication and keep the bundle more maintainable.
- You’re relying on the CSS :has() selector for layout overrides (main:has(.page-with-left-sidebar)), which isn’t fully supported in all browsers—think about a fallback or an alternative way to apply those overrides.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- I noticed you’ve manually added the Research nav link in many HTML templates—extract that into a shared include or macro to keep it DRY and prevent drift when you need to update it in the future.
- The new research.css is quite large; consider splitting it into smaller component or utility files (or reusing existing base/component styles) to reduce duplication and keep the bundle more maintainable.
- You’re relying on the CSS :has() selector for layout overrides (main:has(.page-with-left-sidebar)), which isn’t fully supported in all browsers—think about a fallback or an alternative way to apply those overrides.
## Individual Comments
### Comment 1
<location> `src/_includes/layouts/research-list.njk:15` </location>
<code_context>
+ <article class="article-item">
+ <div class="article-title-line">
+ <h2><a href="{{ post.url | url }}">{{ post.data.title }}</a></h2>
+ {% if post.data.status %}
+ <span class="status-badge status-{{ post.data.status | slug }}">{{ post.data.status }}</span>
+ {% endif %}
+ </div>
</code_context>
<issue_to_address>
Using a slug filter on status may not handle all edge cases.
Sanitize or restrict status values to ensure valid CSS class names and prevent styling issues.
Suggested implementation:
```
<span class="status-badge status-{{ post.data.status | cssclass }}">{{ post.data.status }}</span>
```
You must define a `cssclass` filter in your Eleventy/Nunjucks configuration that:
- Converts the string to lowercase,
- Replaces spaces and invalid characters with hyphens,
- Removes any characters not allowed in CSS class names (e.g., only a-z, 0-9, and hyphens).
Example (in your .eleventy.js config):
```js
eleventyConfig.addFilter("cssclass", function(value) {
return String(value)
.toLowerCase()
.replace(/[^a-z0-9]+/g, '-')
.replace(/^-+|-+$/g, '');
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+15
to
+16
| {% if post.data.status %} | ||
| <span class="status-badge status-{{ post.data.status | slug }}">{{ post.data.status }}</span> |
There was a problem hiding this comment.
suggestion: Using a slug filter on status may not handle all edge cases.
Sanitize or restrict status values to ensure valid CSS class names and prevent styling issues.
Suggested implementation:
<span class="status-badge status-{{ post.data.status | cssclass }}">{{ post.data.status }}</span>
You must define a cssclass filter in your Eleventy/Nunjucks configuration that:
- Converts the string to lowercase,
- Replaces spaces and invalid characters with hyphens,
- Removes any characters not allowed in CSS class names (e.g., only a-z, 0-9, and hyphens).
Example (in your .eleventy.js config):
eleventyConfig.addFilter("cssclass", function(value) {
return String(value)
.toLowerCase()
.replace(/[^a-z0-9]+/g, '-')
.replace(/^-+|-+$/g, '');
});
Comment on lines
4
to
48
| var slider = tns({ | ||
| container: '.project-slider', | ||
| items: 1, // Default items for smallest screens | ||
| items: 1, | ||
| slideBy: 1, | ||
| autoplay: false, | ||
| controls: true, | ||
| // controlsText: ['\2039', '\203A'], // Using CSS ::before for controls now | ||
|
|
||
| nav: false, | ||
| gutter: 15, | ||
| edgePadding: 15, // Default edge padding, adjust for smallest screens | ||
| edgePadding: 15, | ||
| mouseDrag: true, | ||
| loop: true, | ||
| // fixedWidth: 325, // Remove fixedWidth from base for mobile-first approach | ||
|
|
||
| responsive: { | ||
| // Small mobile (e.g. up to 480px) - 1 item, let it fill space | ||
| 320: { // Example breakpoint for small phones | ||
|
|
||
| 320: { | ||
| items: 1, | ||
| edgePadding: 10, | ||
| // fixedWidth: false, // Explicitly ensure fixedWidth is off if inheriting | ||
|
|
||
| }, | ||
| // Larger mobile / Small tablets | ||
| 580: { // Breakpoint where 2 items might start to fit if not fixed width | ||
| items: 1, // Still 1 item if fixedWidth is desired, or 2 if flexible | ||
| // fixedWidth: 260, // Or a slightly smaller fixedWidth if desired | ||
|
|
||
| 580: { | ||
| items: 1, | ||
|
|
||
| edgePadding: 20 | ||
| }, | ||
| // Tablets | ||
|
|
||
| 768: { | ||
| items: 2, | ||
| // fixedWidth: 280, // Example: A fixed width that allows two items | ||
| edgePadding: 20 // Adjust as per layout.css container padding | ||
|
|
||
| edgePadding: 20 | ||
| }, | ||
| // Desktops | ||
|
|
||
| 1024: { | ||
| items: 2, // Or 3 if you prefer and space allows | ||
| fixedWidth: 325, // Re-introduce fixedWidth for larger screens if desired | ||
| edgePadding: 30 // Adjust to match .project-slider-container padding | ||
| items: 2, | ||
| fixedWidth: 325, | ||
| edgePadding: 30 | ||
| }, | ||
| 1200: { // For wider desktops if you want to show more items or wider fixedWidth | ||
| items: 2, // Or 3 | ||
| fixedWidth: 325, // Or adjust as needed | ||
| edgePadding: 50 // Match desktop container padding | ||
| 1200: { | ||
| items: 2, | ||
| fixedWidth: 325, | ||
| edgePadding: 50 | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
There was a problem hiding this comment.
Pull Request Overview
Adds a new “Research” section to the site, including navigation links, page templates, markdown content, styling, and Eleventy configuration updates.
- Introduce “Research” link in site nav (HTML and base layout)
- Create research index and individual post markdown with front-matter
- Add layouts (
research-list.njk,research-post.njk),research.css, and Eleventy collection for research
Reviewed Changes
Copilot reviewed 19 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.html, src/vce/chem/index.html | Insert “Research” nav item |
| src/research/index.md | Add research landing page content |
| src/research/2025/spanish-inquisition.md | Create placeholder research post with lastUpdated |
| src/_includes/layouts/research-list.njk | New layout for listing research articles |
| src/_includes/layouts/research-post.njk | New layout for individual research posts |
| src/_includes/layouts/base.njk | Include research.css and nav update |
| eleventy.config.js | Register research collection and load new layouts |
| src/css/research.css | Styling for research list and posts |
| <main> | ||
| <section id="research-list"> | ||
| <h1>{{ title }}</h1> | ||
| <p class="intro">{{ content | safe }}</p> |
There was a problem hiding this comment.
Wrapping {{ content }} (which itself outputs <p> tags) inside another <p> leads to invalid nested paragraphs. Consider using a <div class="intro">…</div> or output raw content without an extra <p> wrapper.
Suggested change
| <p class="intro">{{ content | safe }}</p> | |
| <div class="intro">{{ content | safe }}</div> |
| description: "A brief look into the Spanish Inquisition" | ||
| date: 2025-06-19 | ||
| layout: layouts/research-post.njk | ||
| permalink: "/research/{{ page.date.getFullYear() }}/{{ page.fileSlug }}/" |
There was a problem hiding this comment.
Front-matter permalink is not evaluated as JavaScript in Eleventy, so {{ page.date.getFullYear() }} will not be replaced. Use Eleventy’s date filter or a computed permalink in config instead (e.g., permalink: "/research/{{ page.date | date: '%Y' }}/{{ page.fileSlug }}/").
Suggested change
| permalink: "/research/{{ page.date.getFullYear() }}/{{ page.fileSlug }}/" | |
| permalink: "/research/{{ page.date | date: '%Y' }}/{{ page.fileSlug }}/" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Introduce a Research subdirectory with its own collection, layouts, and styling in Eleventy, wire it into the navigation, and add sample research content to enable a dedicated research section.
New Features:
Enhancements: