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.

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

refactor: demo account components to oak components - AI-460 #204

Merged
merged 5 commits into from
Oct 11, 2024
Merged

refactor: demo account components to oak components - AI-460 #204

merged 5 commits into from
Oct 11, 2024

Conversation

JBR90
Copy link
Contributor

@JBR90 JBR90 commented Oct 8, 2024 •
edited
Loading

Description

  • Refactors demo components to use Oak Components (excluding model)
  • Refactors header to use Oak components
  • Fixes margin bug on help page (margin was too small with extra demo banner height)

These changes have had some subtle margin / font size changes due to Oak Components spacing and font type constraints.

Issue(s)

Fixes #

How to test

  1. Go to https://oak-ai-lesson-assistant-61gyyikpr.vercel-preview.thenational.academy
    Toggle isDemoUser true in clerk
    Check banner desktop / mobile , check links are working , check pop up for first demo lesson / used all demo lessons up

Screenshots

How it used to look (delete if n/a):
image
image
image
image

How it should now look:
image
image
image
image

Checklist

  • Manually tested across browsers / devices
  • Considered impact on accessibility
  • Does this PR update a package with a breaking change

@vercel Vercel
Copy link

vercel bot commented Oct 8, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oak-ai-lesson-assistant ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 8, 2024 0:43am

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 8, 2024 •
edited
Loading

Playwright e2e tests

Job summary

Download report

To view traces locally, unzip the report and run:

npx playwright show-report ~/Downloads/playwright-report

@JBR90 JBR90 changed the title Refactor demo account components to oak components refactor: demo account components to oak components Oct 8, 2024
@JBR90 JBR90 requested review from a team, annac14 and mikeritson-oak October 8, 2024 12:36
@JBR90 JBR90 marked this pull request as ready for review October 8, 2024 12:40
@JBR90 JBR90 changed the title refactor: demo account components to oak components refactor: demo account components to oak components - AI-460 Oct 8, 2024
@notion-workspace Notion Workspace
Copy link

Demo Account alignment to Oak components

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 8, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mantagen
mantagen approved these changes Oct 8, 2024
View reviewed changes
Copy link
Collaborator

@mantagen mantagen left a comment

Choose a reason for hiding this comment

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

Looks good, just a q around the style prop

apps/nextjs/src/app/aila/help/index.tsx
$maxWidth={"all-spacing-24"}
$ml="auto"
$mr="auto"
style={{ marginTop: marginTop }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Guessing there's a reason we can't use $mt here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maximum is 80 in the oakSpaceBetweenTokens prop - because it's a fixed header, I've had to increase the maximum here so that it's not obscured.

@codeincontext
Copy link
Collaborator

This looks good. I think we should get @mikeritson-oak to check it over before merging

mikeritson-oak
mikeritson-oak approved these changes Oct 11, 2024
View reviewed changes
Copy link
Collaborator

@mikeritson-oak mikeritson-oak left a comment

Choose a reason for hiding this comment

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

All looks good. Tested across browsers and all steps of the demo account flow. 👍

@mikeritson-oak mikeritson-oak merged commit 9f7a3d6 into main Oct 11, 2024
17 checks passed
@mikeritson-oak mikeritson-oak deleted the refactor-demo-account-to-oak-components branch October 11, 2024 08:45
codeincontext added a commit that referenced this pull request Oct 11, 2024
@codeincontext
Revert "Merge pull request #204 from oaknational/refactor-demo-accoun…
8623b54 
…t-to-oak-components"

This reverts commit 9f7a3d6, reversing
changes made to 6737c05.
codeincontext added a commit that referenced this pull request Oct 11, 2024
@codeincontext
fix: revert Oak components for demo UI (#204) (#221)
c6d7d7e
@codeincontext codeincontext restored the refactor-demo-account-to-oak-components branch October 11, 2024 10:54
@codeincontext codeincontext deleted the refactor-demo-account-to-oak-components branch October 11, 2024 10:54
@mikeritson-oak
Copy link
Collaborator

We've had to revert this PR and remove it from the RC build this morning.
Whilst checking over rate limiting on mobile devices a number of issues were spotted with demo account, styling and messaging.

  • Pop-up messaging are all pushed to the bottom of the modal. I assumed these should be centred but on checking production, they seem to be at the top. It might be prudent to check with Anna on styling for these and confirm the desired outcome for mobile.
  • The header messaging seems to be affecting various pages and page elements across the user flow on mobile. See the attached images for examples.

SCR-20241011-kusv
SCR-20241011-ktua-2
SCR-20241011-klem
SCR-20241011-kkrj

@mikeritson-oak mikeritson-oak mentioned this pull request Oct 11, 2024
Merged
@oak-machine-user
Copy link
Collaborator

🎉 This PR is included in version 1.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@JBR90 JBR90 mentioned this pull request Oct 16, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeritson-oak mikeritson-oak mikeritson-oak approved these changes

@mantagen mantagen mantagen approved these changes

@annac14 annac14 Awaiting requested review from annac14

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JBR90 @codeincontext @mikeritson-oak @oak-machine-user @mantagen