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: introduce o-normalise to o-private-foundation #1864

Open
wants to merge 12 commits into
base: 2025-release
Choose a base branch
from

Conversation

frshwtr
Copy link
Contributor

@frshwtr frshwtr commented Nov 6, 2024

Describe your changes

Issue ticket number and link

Link to Figma designs

Checklist before requesting a review

  • I have applied percy label for o-[COMPONENT] or chromatic label for o3-[COMPONENT] on my PR before merging and after review. Find more details in CONTRIBUTING.md
  • If it is a new feature, I have added thorough tests.
  • I have updated relevant docs.
  • I have updated relevant env variables in Doppler.

@frshwtr frshwtr changed the base branch from main to 2025-release-private-foundation November 6, 2024 14:55
@frshwtr frshwtr marked this pull request as ready for review November 6, 2024 15:05
@frshwtr frshwtr requested review from a team as code owners November 6, 2024 15:05
@notlee notlee temporarily deployed to origami-webs-2025-relea-1uhccv November 6, 2024 15:07 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-1uhccv November 6, 2024 15:59 Inactive
@@ -0,0 +1,12 @@
import PrivateFoundation from './src/js/private-foundation.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's delete this and all JS

/// 'body': ('font-smoothing', 'focus', 'reduce-motion')
/// ));
/// @access public
@mixin oNormalise($opts: (
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, no Origami component calls oNormalise so we can delete this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I assumed stories were going to be updated to use private foundation, but it doesn't seem to matter so much in this case.

Copy link
Contributor

@notlee notlee Nov 7, 2024

Choose a reason for hiding this comment

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

Yeah let's exclude demos/devDeps

@@ -0,0 +1,312 @@
/// Visually hide an element while still
/// allowing it to be read by a screenreader
@mixin oNormaliseVisuallyHidden {
Copy link
Contributor

Choose a reason for hiding this comment

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

@import makes all variables, mixins, and functions globally accessible. These will conflict with o-normalise as projects work to migrate to o3.

  1. Which mixins, functions, and variables can we delete? We only need to keep them if they are called by other o2 components.
  2. For those that remain, let's use @use but also rename for good measure. We'll need to make changes to Sass of other components and so it's probably better to keep them separate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I will prefix any of these remaining functions with oPrivate and kept the rest of the name the same.

@@ -4,7 +4,7 @@
@import '@financial-times/o-fonts/main';
@import '@financial-times/o-colors/main';
@import '@financial-times/o-icons/main';
@import '@financial-times/o-normalise/main';
@import '@financial-times/o-private-foundation/o-normalise/main';
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI o-typography, o-buttons, etc will become part of o-private-foundation. Based on changes proposed in other comments, it might be easier to leave their dependency on o-normalise for now until they are subsumed

@notlee notlee temporarily deployed to origami-webs-2025-relea-1uhccv November 7, 2024 10:54 Inactive
@frshwtr frshwtr force-pushed the 2025-release-private-foundation branch from 21d2943 to 631a319 Compare November 7, 2024 10:54
@notlee notlee force-pushed the 2025-release-private-foundation branch from 631a319 to 12ae399 Compare November 7, 2024 10:54
@notlee notlee temporarily deployed to origami-webs-2025-relea-1uhccv November 7, 2024 10:57 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-1uhccv November 7, 2024 10:58 Inactive
Base automatically changed from 2025-release-private-foundation to 2025-release November 7, 2024 11:46
@notlee notlee temporarily deployed to origami-webs-2025-relea-1uhccv November 7, 2024 16:06 Inactive
@@ -1,5 +1,5 @@
@import '@financial-times/o-typography/main';
@import '@financial-times/o-normalise/main';
@financial-times/o-normalise/main
Copy link
Contributor

Choose a reason for hiding this comment

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

oops

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants