Skip to content

[04] [Project Solar / Phase 1 / Showcase] Add support for theming and theme-switching to the showcase #3240

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 0 commits into from
Closed

[04] [Project Solar / Phase 1 / Showcase] Add support for theming and theme-switching to the showcase #3240

wants to merge 0 commits into from

Conversation

@didoo
Copy link
Contributor

@didoo didoo commented Sep 25, 2025 •
edited
Loading

Important

This PR was accidentally closed by wrongly rebasing the parent branch (GitHub saw 0 differences, so it closed this PR). The history of the PR with all its comments is still relevant, but the work will continue in #3390

📌 Summary

See #3390 for details

📋 PCI review checklist
  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
  • If applicable, I've worked with GRC to document the impact of any changes to security controls.
    Examples of changes to controls include access controls, encryption, logging, etc.
  • If applicable, I've worked with GRC to ensure compliance due to a significant change to the in-scope PCI environment.
    Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.

@vercel
Copy link

vercel bot commented Sep 25, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
hds-showcase Ready Ready Preview Nov 27, 2025 2:56pm
hds-website Ready Ready Preview Nov 27, 2025 2:56pm

@didoo didoo mentioned this pull request Sep 25, 2025
Open
5 tasks
@didoo didoo changed the title [WIP] [Project Solaris / Phase 1 / Showcase] Add support for theming and theme-switching to the showcase [WIP] [Project Solis / Phase 1 / Showcase] Add support for theming and theme-switching to the showcase Sep 25, 2025
@didoo didoo changed the title [WIP] [Project Solis / Phase 1 / Showcase] Add support for theming and theme-switching to the showcase [WIP] [Project Solar/ Phase 1 / Showcase] Add support for theming and theme-switching to the showcase Sep 25, 2025
@didoo didoo changed the title [WIP] [Project Solar/ Phase 1 / Showcase] Add support for theming and theme-switching to the showcase [WIP] [Project Solar / Phase 1 / Showcase] Add support for theming and theme-switching to the showcase Sep 25, 2025
@didoo didoo force-pushed the project-solar/phase-1/HDS-5242_showcase/add-themes-support branch from b73dbf9 to a4f41dd Compare September 30, 2025 14:11
@didoo didoo force-pushed the project-solar/phase-1/HDS-5242_showcase/add-themes-support branch from a4f41dd to cc61972 Compare September 30, 2025 14:20
@didoo didoo force-pushed the project-solar/phase-1-main-feature-branch branch from 519a6a6 to 3863dfa Compare September 30, 2025 14:50
@didoo didoo force-pushed the project-solar/phase-1/HDS-5242_showcase/add-themes-support branch from cc61972 to 59418c1 Compare September 30, 2025 16:16
@didoo didoo force-pushed the project-solar/phase-1/HDS-5242_showcase/add-themes-support branch from 59418c1 to 2f20cdc Compare September 30, 2025 17:23
@didoo didoo force-pushed the project-solar/phase-1/HDS-5242_showcase/add-themes-support branch from 2f20cdc to 92910be Compare September 30, 2025 17:23
@didoo didoo force-pushed the project-solar/phase-1/HDS-5242_showcase/add-themes-support branch from 92910be to a1ff2f5 Compare September 30, 2025 20:23
dchyun
dchyun reviewed Nov 24, 2025
View reviewed changes
@didoo didoo force-pushed the project-solar/phase-1/HDS-5505_components/modes-css-compilation branch from 2d244fd to 053e6de Compare November 25, 2025 16:55
didoo added a commit that referenced this pull request Nov 25, 2025
@didoo
fixed small issue with the HdsThemeSwitcher components used in the …
782d036 
…`Theming` showcase page

context: #3240 (comment)
@didoo didoo force-pushed the project-solar/phase-1/HDS-5242_showcase/add-themes-support branch from 1b82973 to e020ded Compare November 25, 2025 16:55
Base automatically changed from project-solar/phase-1/HDS-5505_components/modes-css-compilation to project-solar/phase-1/HDS-5216_modes/modes-feature-branch November 25, 2025 22:17
Base automatically changed from project-solar/phase-1/HDS-5216_modes/modes-feature-branch to project-solar/phase-1-main-feature-branch November 25, 2025 22:40
didoo added a commit that referenced this pull request Nov 25, 2025
@didoo
fixed small issue with the HdsThemeSwitcher components used in the …
deab041 
…`Theming` showcase page

context: #3240 (comment)
@didoo didoo requested review from Copilot and dchyun November 25, 2025 23:33
Copilot AI reviewed Nov 25, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 68 out of 70 changed files in this pull request and generated 7 comments.

didoo added a commit that referenced this pull request Nov 26, 2025
@didoo
converted updatePageStylesheets function to a private method
4c76fe9 
per Copilot suggestion: #3240 (comment)
dchyun
dchyun reviewed Nov 26, 2025
View reviewed changes
Copy link
Contributor

@dchyun dchyun left a comment

Choose a reason for hiding this comment

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

Functionality looks all good to me. Mainly just small things to clean up.

showcase/app/routes/page-foundations/theming/frameless/demo-application-with-theme-switcher.ts
Copy link
Contributor

@dchyun dchyun Nov 26, 2025

Choose a reason for hiding this comment

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

[todo] Every template page should have a route file, so one should be created for page-foundations/theming/index.ts as well.

Copy link
Contributor Author

@didoo didoo Nov 28, 2025

Choose a reason for hiding this comment

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

That's an interesting one! @shleewhite @aklkv currently there is not route file, but the application works and the page is rendered correctly https://hds-showcase-git-project-solar-phase-1hds-5242-97b3b3-hashicorp.vercel.app/foundations/theming and I don't see errors in the browser's console. So why technically is needed?

@didoo didoo closed this Nov 27, 2025
@didoo didoo force-pushed the project-solar/phase-1/HDS-5242_showcase/add-themes-support branch from 0a1d9c5 to c1daff5 Compare November 27, 2025 14:52
@didoo
Copy link
Contributor Author

didoo commented Nov 27, 2025

@dchyun @KristinLBradley unfortunately a wrong origin definition in my local Git client made so that I pushed to the wrong branch (the parent) instead of this one, which made GitHub see 0 commits in this branch and close the PR, and now it can't be re-opened. I had to create a new PR #3390 and we'll continue the code review there, while at the same time I'll reply to your review comments in this one. Hopefully it should not be major changes, so I think it's doable. Sorry for the extra work in having to review in one and comment in another.

@didoo didoo mentioned this pull request Nov 27, 2025
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dchyun dchyun dchyun left review comments

@KristinLBradley KristinLBradley KristinLBradley left review comments

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

packages/components project solar showcase

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@didoo @dchyun @KristinLBradley @hashibot-hds