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

Fix: Remove color styles from typography variation controls - #61217 #61280

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Fix: Remove color styles from typography variation controls - #61217 #61280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/edit-site/src/components/global-styles/preview-iframe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export default function PreviewIframe( {
label,
isFocused,
withHoverView,
addGlobalStyles = true,
} ) {
const [ backgroundColor = 'white' ] = useGlobalStyle( 'color.background' );
const [ gradientValue ] = useGlobalStyle( 'color.gradient' );
Expand Down Expand Up @@ -129,7 +130,10 @@ export default function PreviewIframe( {
style={ {
height: normalizedHeight * ratio,
width: '100%',
background: gradientValue ?? backgroundColor,
color: addGlobalStyles ? undefined : '#000',
background: addGlobalStyles
? gradientValue ?? backgroundColor
: '#fff',
cursor: withHoverView ? 'pointer' : undefined,
} }
initial="start"
Expand Down
5 changes: 4 additions & 1 deletion packages/edit-site/src/components/global-styles/screen-typography.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ function ScreenTypography() {
/>
<div className="edit-site-global-styles-screen">
<VStack spacing={ 7 }>
<TypographyVariations title={ __( 'Presets' ) } />
<TypographyVariations
addGlobalStyles={ false }
title={ __( 'Presets' ) }
/>
{ ! window.__experimentalDisableFontLibrary &&
fontLibraryEnabled && <FontFamilies /> }
<TypographyElements />
Expand Down
7 changes: 6 additions & 1 deletion packages/edit-site/src/components/global-styles/variations/variations-typography.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ import PreviewIframe from '../preview-iframe';
import Variation from './variation';
import Subtitle from '../subtitle';

export default function TypographyVariations( { title, gap = 2 } ) {
export default function TypographyVariations( {
title,
gap = 2,
addGlobalStyles = false,
} ) {
const typographyVariations = useTypographyVariations();

if ( ! typographyVariations?.length ) {
Expand All @@ -37,6 +41,7 @@ export default function TypographyVariations( { title, gap = 2 } ) {
<Variation key={ index } variation={ variation }>
{ ( isFocused ) => (
<PreviewIframe
addGlobalStyles={ addGlobalStyles }
label={ variation?.title }
isFocused={ isFocused }
>
Expand Down
1 change: 1 addition & 0 deletions packages/edit-site/src/components/sidebar-navigation-screen-global-styles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ function SidebarNavigationScreenGlobalStylesContent() {
) }
{ typographyVariations?.length && (
<TypographyVariations
addGlobalStyles
Copy link
Contributor Author

@hbhalodia hbhalodia May 1, 2024 •
edited

Choose a reason for hiding this comment

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

Hello @richtabor,

I am passing this prop as true here, because I am thinking we should keep the color combination in the sidebar navigation global screen, Let me know if we need to remove the color from here as well, I will update the PR.

Thank You.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove it here.

Copy link
Contributor Author

@hbhalodia hbhalodia May 23, 2024 •
edited

Choose a reason for hiding this comment

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

Sure, I will work on this PR and update it by removing the color from here as well.

So after update, both the global screen and side panel would not have the color effect in the typography. Am I correct?

Hence, I am guessing that, we should remove the props from all places, if we make it consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I had a different idea for how to do it which I tried here: #61898

I'm not sure which option is better, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, looks like the good option. Just add the styles to iframe whatever we want using this prop.

title={ __( 'Typography' ) }
gap={ gap }
/>
Expand Down