This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Ensure Cart block supports the WCBlock level global styles for Blocks » Product Title
#8939
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add a span with product title class to fix the global style of product title. - Inherit font-weight for product title.
tarunvijwani
changed the title
Blocks » Product TitleBlocks » Product Title
tarunvijwani
changed the base branch from
trunk
to
add/global-style-for-cart-checkout
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
tarunvijwani
added
type: enhancement
|
Size Change: +4.76 kB (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
…line-items-product-title-global-styles
woocommercebot
requested review from
a team and
nielslange
and removed request for
a team
tarunvijwani
requested review from
a team,
Aljullu,
nielslange and
mikejolley
and removed request for
nielslange and
a team
…line-items-product-title-global-styles
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…line-items-product-title-global-styles
…line-items-product-title-global-styles
…line-items-product-title-global-styles
…line-items-product-title-global-styles
…line-items-product-title-global-styles
mikejolley
reviewed
Apr 6, 2023
| name={ name } | ||
| permalink={ permalink } | ||
| /> | ||
| <span className="wc-block-components-product-title"> |
There was a problem hiding this comment.
I believe wrapping this with a classname is the wrong approach. It looks as though wc-block-components-product-title is used within the product title block and is not exposed as a component. Why don't we expose a component instead? if the styles and component are to be shared, that makes the most sense.
- Create a base component called product title
- Move the code from the product title block into the component. Note that it accepts
tagso you can define the h* tag or pass a span. - Make both the product title block and the cart item list use the new component.
There was a problem hiding this comment.
I believe wrapping this with a classname is the wrong approach. It looks as though
wc-block-components-product-titleis used within the product title block and is not exposed as a component. Why don't we expose a component instead? if the styles and component are to be shared, that makes the most sense.
- Create a base component called product title
- Move the code from the product title block into the component. Note that it accepts
tagso you can define the h* tag or pass a span.- Make both the product title block and the cart item list use the new component.
Thanks for your review, @mikejolley. Creating the base component sounds like a good approach. I'll do that now.
Aljullu
reviewed
Apr 6, 2023
There was a problem hiding this comment.
I tested and it works well, but I have to say I'm a bit unsure if we should go ahead with this. Let me explain:
The Product Title global styles should be applied to all Product Title blocks, but nothing else IMO. In the case of the Cart block, those product titles are not the Product Title block. Instead, they are a component displaying the product title. I'm wary of applying the styles from a block to a different component, as that might cause future issues and be confusing for users. For example, it doesn't allow overriding the global styles: ie, imagine I want product titles to be bold except when displayed in the Cart block, I wouldn't be able to do that with this approach.
Something similar happened with the Sale Badge. We have the Sale Badge block and the sale badge component rendered inside the Product Image block. Currently, styles applied to the Sale Badge block are applied to the sale badge component. But IMO that's confusing and prone to break in the future.
I posted a question regarding this in #8151.
|
Thanks for your review, @Aljullu. In #8939 (review), @mikejolley suggested creating a base component called product title and using that instead of applying styles from a different block. I'll keep working on this approach now. |
…line-items-product-title-global-styles
nielslange
reviewed
Apr 11, 2023
Comment on lines
+33
to
+41
| export const CHECKOUT_PAGE_ID = STORE_PAGES.checkout?.id; | ||
| export const CHECKOUT_URL = STORE_PAGES.checkout?.permalink; | ||
| export const PRIVACY_URL = STORE_PAGES.privacy?.permalink; | ||
| export const PRIVACY_PAGE_NAME = STORE_PAGES.privacy?.title; | ||
| export const TERMS_URL = STORE_PAGES.terms?.permalink; | ||
| export const TERMS_PAGE_NAME = STORE_PAGES.terms?.title; | ||
| export const CART_PAGE_ID = STORE_PAGES.cart?.id; | ||
| export const CART_URL = STORE_PAGES.cart?.permalink; | ||
| export const LOGIN_URL = STORE_PAGES.myaccount?.permalink |
There was a problem hiding this comment.
These changes ensure that the preview does not break in Storybook.
|
We're discussing the final approach on slack: p1681203933835379-slack-C02UBB1EPEF |
|
Closing the PR as per the discussion on Slack: p1681728763577949/1681203933.835379-slack-C02UBB1EPEF The Product Title block and Product Title component in the Cart Line Items block are different; hence they should have different global style options. The Product Title component should inherit the parent's Global styles, i.e., Cart Line Items inner block instead of the Product Title block. |
nielslange
deleted the
fix/8829-cart-line-items-product-title-global-styles
branch
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8829
Fixes #8830
Fixes #8831
Currently, the Cart block partially supports the WCBlock level global styles for Blocks » Product Title.
This PR ensures the Cart block fully supports the WCBlock level global styles.
Screenshots
Blocks » Product Title » Typography
Blocks » Product Title » Color
Blocks » Product Title » Layout
Testing
Automated Tests
User Facing Testing
WP Admin » Settings » Reading.WP Admin » Apperance » Editor.Stylessidebar.Blockssection.Product Titleblock.Blocks » Product Title » Typographysettings.FONT,SIZE,FONT WEIGHT,LINE HEIGHTandLETTER SPACE.Blocks » Product Title » Colorssettings.BackgroundandTextcolors. (Note thatHeadingscolor is not used in this block.)Blocks » Product Title » Layoutsettings.MARGIN.Note: Cart product title styles should match the product titles in the products block so that product titles look consistent throughout the site.
WooCommerce Visibility
Changelog