Skip to content
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.

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

feat(PPDSC-2859): use newskit-website theme from publisher #718

Merged
merged 38 commits into from
Apr 19, 2023

Conversation

LukeFinch
Copy link
Contributor

@LukeFinch LukeFinch commented Mar 21, 2023

PPDSC-2859

What

Background - why this is needed
Uses themes from the newskit-themes-publisher repo for the docs site.

What did you do

  • Import website themes from the publisher
  • Remove old website themes data
  • Update tests, we no longer need to check for overrides, as they are contained in the theme package
  • Changed some on-site components to line up with expectations from design

What does the reviewers should expect
Design changes have been manually checked and approved
Snapshot updates are mostly due to px changing to rem & hex codes coming in lower case
I've removed a test

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

@LukeFinch LukeFinch added the WIP Work in progress label Mar 21, 2023
@LukeFinch LukeFinch requested a review from a team as a code owner March 21, 2023 15:35
@github-actions github-actions bot added the feature This change contains a new feature label Mar 21, 2023
@LukeFinch LukeFinch added the DO NOT MERGE Do not merge this PR label Mar 31, 2023
@LukeFinch LukeFinch changed the title feat(PPDSC-2859): WIP - use newskit-website theme from publisher feat(PPDSC-2859): use newskit-website theme from publisher Apr 6, 2023
@LukeFinch LukeFinch added ready for design review Please assist in getting this reviewed Requires design review and removed DO NOT MERGE Do not merge this PR WIP Work in progress labels Apr 6, 2023
@LukeFinch LukeFinch added ready for review Please assist in getting this reviewed and removed ready for design review Please assist in getting this reviewed Requires design review labels Apr 14, 2023

export const docsThemeLight = createTheme({
name: 'docs-theme-light',
overrides: lightOverrides,
baseTheme: newskitLightTheme, // TODO: Use newsKitTheme from Publisher eventually
Copy link
Contributor

@Vanals Vanals Apr 14, 2023

Choose a reason for hiding this comment

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

Should you make ticket for this? in case we forget?
Sounds an extra good step for testing how user would implement it.

Even if, in this scenario I think we do not need to specify newskitLight as it is the default base theme inside the function, am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add it to the backlog 👍

@Vanals
Copy link
Contributor

Vanals commented Apr 14, 2023

Left a few comments but the rest looks good. If wasn't checked already could you also double check the SVG Previewer is behaving as expected? it uses all the doc site theme to preview the SVGs. It should correctly import and use the themes for displaying the SVG.
See colors-theme-list.tsx

@LukeFinch
Copy link
Contributor Author

Left a few comments but the rest looks good. If wasn't checked already could you also double check the SVG Previewer is behaving as expected? it uses all the doc site theme to preview the SVGs. It should correctly import and use the themes for displaying the SVG. See colors-theme-list.tsx

good callout - I will double-check.
In theory - if it's importing the exports from docs-theme.ts it should be fine

package.json Outdated Show resolved Hide resolved
site/theme/doc-theme.ts Outdated Show resolved Hide resolved
@LukeFinch LukeFinch linked an issue Apr 14, 2023 that may be closed by this pull request
@jps jps requested a review from Vanals April 19, 2023 09:57
@Vanals
Copy link
Contributor

Vanals commented Apr 19, 2023

LGTM

@jps jps merged commit f854412 into main Apr 19, 2023
@jps jps deleted the feat/PPDSC-2859-use-theme-from-publisher branch April 19, 2023 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This change contains a new feature ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement newskit-site theme Publisher package in NewsKit repo
4 participants