This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Add style controls for order shipping and billing address blocks #10633
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
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
assets/js/blocks/order-confirmation/billing-wrapper/index.tsx
assets/js/blocks/order-confirmation/downloads/edit.tsx assets/js/blocks/order-confirmation/downloads/index.tsx assets/js/blocks/order-confirmation/shipping-wrapper/index.tsx assets/js/blocks/order-confirmation/status/index.tsx assets/js/blocks/order-confirmation/summary/edit.tsx assets/js/blocks/order-confirmation/summary/index.tsx assets/js/blocks/order-confirmation/totals/edit.tsx assets/js/blocks/order-confirmation/totals/index.tsx assets/js/blocks/shared/scripts/migration-from-product-collection-to-products.tsx assets/js/blocks/shared/scripts/migration-from-products-to-product-collection.tsx assets/js/editor-components/product-tag-control/index.js |
|
Size Change: +409 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
nielslange
reviewed
Aug 17, 2023
There was a problem hiding this comment.
Thanks for working on this, @tarhi-saad. Below, I left a few comments.
Apart from the comments, I wonder why this PR is so big. I wonder because other PRs that added style controls, only touched a handful of files, e.g. https://github.com/woocommerce/woocommerce-blocks/pull/10185/files.
| @@ -0,0 +1,9 @@ | |||
| .wc-block-order-confirmation-shipping-address { | |||
| address { | |||
| width: 100% !important; | |||
There was a problem hiding this comment.
Why are we using width: 100% !important; instead of width: 100%; here? I'm asking as !important should only be used as last option. In this case, even when removing width: 100% !important; completely, I do not see a change on the site.
assets/js/blocks/order-confirmation/shipping-address/style.scss
Outdated
Show resolved
Hide resolved
Comment on lines
9
to
14
| a { | ||
| margin-right: $gap; | ||
| } | ||
| a.has-link-color { | ||
| color: inherit; | ||
| } |
There was a problem hiding this comment.
Shouldn't we fully utilise SCSS in the following case:
Suggested change
| a { | |
| margin-right: $gap; | |
| } | |
| a.has-link-color { | |
| color: inherit; | |
| } | |
| a { | |
| margin-right: $gap; | |
| &.has-link-color { | |
| color: inherit; | |
| } | |
| } |
Comment on lines
23
to
26
| // Remove style because its handled by the server-side render. | ||
| const { style, ...blockProps } = useBlockProps( { | ||
| className: 'wc-block-order-confirmation-summary', | ||
| } ); |
There was a problem hiding this comment.
Can you explain this part? The comment says "Remove style [...]", but the code looks like we're adding a class.
Comment on lines
18
to
20
| border-style: solid; | ||
| border-color: currentColor; | ||
| border-width: 0 0 1px 0; |
There was a problem hiding this comment.
We could use the shorthand syntax here:
Suggested change
| border-style: solid; | |
| border-color: currentColor; | |
| border-width: 0 0 1px 0; | |
| border: 0 0 1px 0 solid currentColor; |
Comment on lines
169
to
251
| /** | ||
| * Get the frontend script handle for this block type. | ||
| * | ||
| * @param string $key Data to get, or default to everything. | ||
| */ |
There was a problem hiding this comment.
The return type seems to be missing here:
Suggested change
| /** | |
| * Get the frontend script handle for this block type. | |
| * | |
| * @param string $key Data to get, or default to everything. | |
| */ | |
| /** | |
| * Get the frontend script handle for this block type. | |
| * | |
| * @param string $key Data to get, or default to everything. | |
| * @return null | |
| */ |
| /** | ||
| * Get a fake order for previews. | ||
| * | ||
| * @return \WC_Order Fake order. |
There was a problem hiding this comment.
There's a fullstop behind the return type. This is only required for comments, not for return values.
Suggested change
| * @return \WC_Order Fake order. | |
| * @return \WC_Order Fake order |
| protected function render_content( $order, $permission = false, $attributes = [] ) { | ||
| if ( ! $permission ) { | ||
| // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment | ||
| return '<p>' . wp_kses_post( apply_filters( 'woocommerce_thankyou_order_received_text', esc_html__( 'Thank you. Your order has been received.', 'woo-gutenberg-products-block' ), null ) ) . '</p>'; |
There was a problem hiding this comment.
What are your thoughts on formatting this section similar to the next one for readability-reasons:
Suggested change
| return '<p>' . wp_kses_post( apply_filters( 'woocommerce_thankyou_order_received_text', esc_html__( 'Thank you. Your order has been received.', 'woo-gutenberg-products-block' ), null ) ) . '</p>'; | |
| return '<p>' . wp_kses_post( | |
| apply_filters( | |
| 'woocommerce_thankyou_order_received_text', | |
| esc_html__( 'Thank you. Your order has been received.', 'woo-gutenberg-products-block' ), | |
| null | |
| ) | |
| ) . '</p>'; |
| * @return string | ||
| */ | ||
| protected function render_content_fallback() { | ||
| // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment |
There was a problem hiding this comment.
Can you explain the purpose of // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment in this case? It seems there's no hook that needs commenting.
| * @return string | ||
| */ | ||
| protected function render_summary_row( $name, $value ) { | ||
| return $value ? '<li class="wc-block-order-confirmation-summary-list-item"><span class="wc-block-order-confirmation-summary-list-item__key">' . esc_html( $name ) . '</span> <span class="wc-block-order-confirmation-summary-list-item__value">' . wp_kses_post( $value ) . '</span></li>' : ''; |
There was a problem hiding this comment.
What are your thoughts on splitting the code into multiple lines to increase the readability:
Suggested change
| return $value ? '<li class="wc-block-order-confirmation-summary-list-item"><span class="wc-block-order-confirmation-summary-list-item__key">' . esc_html( $name ) . '</span> <span class="wc-block-order-confirmation-summary-list-item__value">' . wp_kses_post( $value ) . '</span></li>' : ''; | |
| if ( $value ) { | |
| return '<li class="wc-block-order-confirmation-summary-list-item">' | |
| . '<span class="wc-block-order-confirmation-summary-list-item__key">' | |
| . esc_html( $name ) | |
| . '</span> ' | |
| . '<span class="wc-block-order-confirmation-summary-list-item__value">' | |
| . wp_kses_post( $value ) | |
| . '</span>' | |
| . '</li>'; | |
| } else { | |
| return ''; | |
| } |
|
@tarhi-saad please change the base from trunk to the feature branch. That will cause the diff to be smaller (cc @nielslange) |
|
@nielslange, @mikejolley, I changed the base branch! Now you can see only my changes! |
|
@tarhi-saad the default has padding but no border now:
I think it would be good to add spacing controls (to control padding) if we're leaving the border off. I wonder if its also possible to set defaults, so we have the border and padding (as originally designed) by default? |
|
@mikejolley, would you mind reviewing the adjustments? I've moved the |
nielslange
reviewed
Aug 18, 2023
There was a problem hiding this comment.
The changes work as expected and the requirements of #10120 are met.
However, I noticed that the PR introduces various TS errors, which might be coming from the underlying branch feature/blockify-order-confirmation. In addition, all E2E tests and the JS Unit Tests are failing. Before merging this PR, we should ensure that all tests are passing.
|
@nielslange, thank you for the review! 🙌
This PR is set to merge into the feature branch. We can proceed with the merge even if the E2E test fails, as we intend to address those failures within the feature branch itself. (See @mikejolley's status update: pdFofs-17q-p2#comment-1590) |
Sounds good, as long as we ensure all tests are green before merging the featured branch into trunk. |
The wrapper border will be the one being updated. So, having another inner border is a styling issue!
83f6e24 to
36a7abf
Compare
tarhi-saad
added a commit
that referenced
this pull request
Aug 25, 2023
mikejolley
pushed a commit
that referenced
this pull request
Sep 1, 2023
mikejolley
pushed a commit
that referenced
this pull request
Sep 19, 2023
mikejolley
added a commit
that referenced
this pull request
Sep 20, 2023
* Main block types for confirmation * Initial blocks * Styling and placeholders * Make blocks experimental * Update summary icon * Add name/description for status block and missing text descriptions in the block. Closes #10057 * Order confirmation: Convert Order Details Templates to Blocks (#10095) * Move code from templates into the details block * Details -> Totals * Downloads block * Sample content for downloads block * Add block icon * Add conversion template (#10077) * Update inner block name * Add default title constant * Revert "Add default title constant" This reverts commit 1dd3bbf. * Add global styles for order confirmation status block (#10164) * Implement style controls * Prevent link color spilling over onto wrapper * Add styles and remove class names * Remove __experimentalWritingMode * Add global styles for order confirmation summary block (#10179) * Styles for summary * Remove __experimentalWritingMode * Add table styles for order details (#10185) * Add table styles for order details * __experimentalFontWeight * Add link styles * Handle preview link styles * Unauthenticated views for Order Confirmation template (#10414) * Different views by permission * check user id matches when logged out * Add order confirmation wrapper block (#10286) * Add a heading wrapper block * Register the BillingWrapper Block server side * Fix exception 'render_content' error * Add the Billing Wrapper Block to the template * Fix wrong block name error * Fix php error * Conditionally render Billing Address within the Wrapper * Fix parent rendering * Clean up code (remove billing address from the template) * Update titles, descriptions, and icons of the billing Block and inner block * Fix broken block by removing the "parent" keyword * Use a user-friendly title and description for the Billing Wrapper * Update Billing Wrapper Block's title case Co-authored-by: Mike Jolley <mike.jolley@me.com> * Fix PHP failing unit test --------- Co-authored-by: Mike Jolley <mike.jolley@me.com> * Remove "thanks" for authenticated page * Introduce shipping wrapper based on billing wrapper Closes #10053 * Order confirmation block: Verify email address for guest customers (#10567) * Add verify step for guest orders * Render content to pass through block content * Revert package changes * Customer orders cannot use email to verify * Add style controls for order shipping and billing address blocks (#10633) * Order confirmation block styling (#10780) * Add missing heading to order details * Summary block spacing * Update css variables * table styles * Inherit border styles for cells * Alignment and address styles * Add downloads wrapper * Style controls * Fix typo * Update Download Wrapper's Icon * Fix TS error * Disable Download Block's server side rendering in the editor This fixes the loading after each style change from the style controls * Clean up Downloads render functions * Fix client side Downloads Block's table border * Download + Total wrappers and tables styling * small screen * Remove server side render for previews --------- Co-authored-by: Saad Tarhi <saad.trh@gmail.com> * Shorten template description * Update test address data * Avoid leaking order key in permission form * Remove todo * Make email form required. * Remove edit page link * Remove empty columns from address wrapper * Remove IIFE * typo * Update description to mention billing * Adjust link scss * Fix wrapper markup and spacing controls * Add link preview in editor * Add initial E2E setup for the Order Confirmation Block (#10840) * Fix WC_DateTime check * Move form outside of block markup * Add additional information block (#10842) * Add block which contains hooks * Use skeleton for placeholder instead of illustration * Remove duplicate methods * Remove duplicate align tag * Update meta styles * Tests for order confirmation conditional blocks (#10972) * Add tests for conditional blocks * Move setup into test * Add E2E to the the Order Confirmation Block (#10863) * Add editor util functions * Update editor template E2E test * Add the "exact" property for consistency * Skip test Can't get the element in the page. More investigation needed! Skipping for now. * Fix "transformIntoBlocks" logic * Add tests for logged in user * Fix "beforeAll" config * Confirm downloads section is visible when logged in * Create "verifyOrderConfirmationDetails" util function * Add logged in test case * Add Guest user test case * Fix editor e2e testing * Apply a proper teardown * Fix failing tests after logout * Ensure we are logged in before visiting the editor * Ensure to have shipping selected * Wait for changes to be saved on the editor * Ensure shipping options is selected * Remove comment * Ensure we are logged in before going to the admin page * Mark the Order Confirmation as a side effect test * OrderConfirmation blocks are not experimental * resolve merge conflict * Revert package lock changes * Fix enqueue_assets * Fix CSS 404s * Make template tests more robust * Fix page URL for default confirmation page * Try afterEach to log back in * Skip guest/logout use cases Login out causes other tests to fail. We will implement these case when the multiple sign in roles are introduced in the codebase. * Remove tests requiring login out & add comments * Remove unused util functions * Hide confirmation blocks from post editor --------- Co-authored-by: Saad Tarhi <saad.trh@gmail.com> Co-authored-by: Paulo Arromba <17236129+wavvves@users.noreply.github.com>
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.
In this PR, we added style controls (i.e., Typography, colors, border styles) on the settings sidebar of the
Shipping AddressandBilling Addressblocks.Fixes #10120
Other Checks
[type]label or a[skip-changelog]label.Testing
User Facing Testing
SettingssidebarBilling AddressorShipping Addressinner blockColor,Typography, andBorderstyle controls are availableWooCommerce Visibility
Performance Impact