Skip to content

Conversation

@zeroedin
Copy link
Contributor

@zeroedin zeroedin commented Aug 20, 2025

What I did

  1. Corrected lightdom(-shim).css routing in dev server for downstream use cases
  2. Improved trailing slash behavior in dev server

Testing Instructions

  1. Open dev server, add -lightdom.css to a component and link in a demo. Ensure lightdom css is properly linked.

Notes to Reviewers

@zeroedin zeroedin requested a review from bennypowers August 20, 2025 13:57
@zeroedin zeroedin self-assigned this Aug 20, 2025
@zeroedin zeroedin added the bug label Aug 20, 2025
@changeset-bot
Copy link

changeset-bot bot commented Aug 20, 2025

⚠️ No Changeset found

Latest commit: 21e0a14

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@zeroedin zeroedin changed the title fix(tools): correct lightdom css routing, improve trailing slash beha… fix(tools): correct lightdom css routing, improve trailing slash behavior Aug 20, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Aug 20, 2025

✅ Commitlint tests passed!

More Info
{
  "valid": true,
  "errors": [],
  "warnings": [],
  "input": "fix(tools): correct lightdom css routing, improve trailing slash behavior"
}

decodeURIComponent(demo.source?.href.replace(options.sourceControlURLPrefix, '') ?? '');
// split the demoSource into an array of parts
const demoSourceParts = demoSource.split('/');
// if demoSourceParts contains options.elementsDir, then build the filePath from the rootDir and the demoSource
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a naive change. Perhaps we should only check if the first part of the demo source matches the elementsDir? @bennypowers thoughts?

@zeroedin
Copy link
Contributor Author

zeroedin commented Aug 21, 2025

npm run docs fails on main branch as well right now for the same error.

[11ty] Problem writing Eleventy templates:
[11ty] 1. Having trouble rendering njk template ./docs/quick-start.md (via TemplateContentRenderError)
[11ty] 2. (./docs/quick-start.md)
[11ty] EleventyNunjucksError: Error with Nunjucks paired shortcode generateImportMap (via Template render error)
[11ty] 3. Cannot read properties of undefined (reading 'replace') (via TypeError)

[11ty] Original error stack trace: TypeError: Cannot read properties of undefined (reading 'replace')
[11ty] at Generator.htmlInject (file:///Users/sspriggs/Sites/_patternfly/patternfly-elements/node_modules/@jspm/generator/dist/generator-c35f6b68.js:4946:236)
[11ty] at async Object. (/Users/sspriggs/Sites/_patternfly/patternfly-elements/docs/_plugins/create-import-map.cjs:12:18)
[11ty] Copied 5259 Wrote 0 files in 5.40 seconds (v3.1.2)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 21, 2025

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit 2667d1a
😎 Deploy Preview https://deploy-preview-2944--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 21, 2025

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit 21e0a14
🔍 Latest deploy log https://app.netlify.com/projects/patternfly-elements/deploys/68af4f9d4e25ab0008553b30
😎 Deploy Preview https://deploy-preview-2944--patternfly-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions
Copy link
Contributor

SSR Test Run for f1357ba: Report

@github-actions
Copy link
Contributor

SSR Test Run for 2667d1a: Report

Copy link
Collaborator

@adamjohnson adamjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno if you wanna also try tackling adding a trailing slash to non-index.html demos. Otherwise, LGTM 👏 .

Comment on lines +47 to +53
/**
* Ensures trailing slash for component URLs
* FROM: `/elements/footer`
* TO: `/elements/footer/`
* @param config normalized PFE dev server config
*/
const ensureTrailingSlashMiddleware: PfeMiddleware = () => (ctx, next) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a minor nitpick, but if:

http://localhost:8000/components/jump-links

will add the trailing /, would it also be worthwhile to add the trailing slash to the other demos as well? eg:

http://localhost:8000/components/jump-links/demo/centered-list
...becomes...
http://localhost:8000/components/jump-links/demo/centered-list/

Feel free to resolve if this is out of scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the trailing slashes themselves were kinda a nitpick of my own, so why not solve one more case.

@adamjohnson
Copy link
Collaborator

This probably needs a changeset.

@bennypowers
Copy link
Member

bennypowers commented Sep 9, 2025

Sorry for the delay in review. Please consider that an indication of how ill-advised my original approach is, moreso than as a comment on your changes

That being said, I have questions:

  • has this been tested on Windows?
  • what is this coming to solve? Im still not clear on what the exact problem is that this addresses

@zeroedin
Copy link
Contributor Author

zeroedin commented Sep 9, 2025

That being said, I have questions:

  • has this been tested on Windows?
  • what is this coming to solve? Im still not clear on what the exact problem is that this addresses
  • Has not been tested on Windows. I'm not sure why it would be any different, but I will test.
  • Solves the fact that in the dev server, the -lightdom.css files literally do not load downstream in RHDS given the current state of things.

* TO: `elements/pf-jazz-hands/pf-jazz-hands-lightdom.css` or `elements/pf-jazz-hands/pf-jazz-hands-lightdom-shim.css`
* @param config normalized PFE dev server config
*/
const lightdomShortPathMiddleware: PfeMiddleware = config => (ctx, next) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be hardcoded at the pfe level? IIRC we don't have lightdom stylesheets here. It would be better to implement this middleware in rhds's wds config

@zeroedin
Copy link
Contributor Author

Closed in favor of downstream fix. RedHat-UX/red-hat-design-system#2663

@zeroedin zeroedin closed this Oct 13, 2025
@zeroedin zeroedin deleted the fix/tools/lightdom-css-routing branch October 13, 2025 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AT passed Automated testing has passed bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants