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(731) migrate newskit demo to docs site #870

Merged
merged 25 commits into from
May 16, 2023
Merged

feat(731) migrate newskit demo to docs site #870

merged 25 commits into from
May 16, 2023

Conversation

mutebg
Copy link
Contributor

@mutebg mutebg commented May 3, 2023

closes #731

What

  1. Background - migrate newskit demo from separate repo to newskit repo under docs folder
  2. What did you do:
  • Replaced legacy components like Stack, Grid, Card and AudioPlayer with GridLayout, CardComposable and AudioPlayerComposable in the code examples
  • Updated demo template to use GridLayout and made it mobile friendly
  • Added themes directly from the publisher instead of local ones.
  • Update the fetch-fonts script to download the licensed fonts for storybook and docs.
  1. What does the reviewers should expect -> go to http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/731-add-demo/demo/ and checkout the demo.

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:

@github-actions github-actions bot added the feature This change contains a new feature label May 3, 2023
@mutebg mutebg changed the title feat(731) add newskit demo to docs site feat(731) migrte newskit demo to docs site May 3, 2023
@pp-serviceaccount
Copy link
Collaborator

@mutebg mutebg marked this pull request as ready for review May 4, 2023 03:11
@mutebg mutebg added ready for review Please assist in getting this reviewed ready for design review Please assist in getting this reviewed labels May 4, 2023
@JohnTParsons JohnTParsons changed the title feat(731) migrte newskit demo to docs site feat(731) migrate newskit demo to docs site May 4, 2023
JohnTParsons
JohnTParsons previously approved these changes May 4, 2023
@jps
Copy link
Contributor

jps commented May 4, 2023

I get an issue with the fonts because I'm in the dark theme, also there is no way to change out of it, not sure if this is something we want to fix in this ticket though.
image

site/public/static/demo/page7.tsx Show resolved Hide resolved
site/pages/demo/page7.tsx Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Vanals
Copy link
Contributor

Vanals commented May 4, 2023

LGTM, is the dw theme gonna be changed in this ticket?

Also in a follow up ticket we could add the font metrics, for proper TextCropping, and I was wondering if we have a video of the DEMO? like someone running over it? Would be nice if we could include it in the home pace

@mutebg
Copy link
Contributor Author

mutebg commented May 4, 2023

LGTM, is the dw theme gonna be changed in this ticket?

I am waiting for newscorp-ghfb/newskit-themes-publisher#108 so that I can add the rest of the themes

@mutebg mutebg requested review from jps and JohnTParsons May 12, 2023 13:09
JohnTParsons
JohnTParsons previously approved these changes May 15, 2023
@mutebg mutebg removed the ready for design review Please assist in getting this reviewed label May 15, 2023
Vanals
Vanals previously approved these changes May 15, 2023
@mutebg mutebg dismissed stale reviews from Vanals and JohnTParsons via 2da3fd2 May 15, 2023 15:17
Copy link
Collaborator

@JohnTParsons JohnTParsons left a comment

Choose a reason for hiding this comment

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

Nice work, Stoyan

@mutebg mutebg merged commit 046e688 into main May 16, 2023
@mutebg mutebg deleted the feat/731-add-demo branch May 16, 2023 09:25
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.

Migrate NewsKit Demo into the main NewsKit Repo
5 participants