Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Store details step - add contextual information and skip link #4735

Closed
wants to merge 6 commits into from

Conversation

samueljseay
Copy link
Contributor

@samueljseay samueljseay commented Jul 2, 2020

Fixes #4566

Changes:

  • Updated @wordpress/components and @wordpress/base-styles
  • Use the new Card and Flex from @wordpress/components
  • Add contextual tooltips
  • Adjust existing styles to match new designs

Accessibility

Screenshots

Screenshot 2020-07-02 at 3 58 35 PM

Screenshot 2020-07-02 at 3 58 48 PM

Detailed test instructions:

  • Start the setup wizard via the usual means (either after installing WooCommerce the first time or via WooCommerce > Settings > Help > Setup Wizard)
  • Visually confirm that the page matches the designs
  • Visually confirm that the tooltips work on hover and that the content is correct.
  • You should also test that changing a user's color scheme still works. There are some good instructions on testing that here


class StoreDetails extends Component {
constructor( props ) {
super( ...arguments );
super( props );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was just a formatting change, really no need to pass anything but props to Component.

</H>
<H className="woocommerce-profile-wizard__header-subtitle">
{ __(
'This will help us configure your store and get you started quickly',
"Tell us about your store and we'll get you set up in no time",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A question: do I need to do anything for translations in other languages here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, its wrapped in __(), so the translations are handled elsewhere.

validate={ validateStoreAddress }
<Tooltip
text={ __(
'Your store address will help us configure currency\n options and shipping rules automatically.\n This information will not be publicly visible and can\n easily be changed later.',
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 used newlines and whitespace: pre to get this displaying as close to the designs as possible. This seems problematic for translations?

I tried modifying elements on popover more with styles, but it was quite constrained, I could not retain the exact width of the design.

) }
</Form>
</Card>
<CardFooter>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making use of the new Card, CardFooter etc from @wordpress/components.

I've used 2 footers here to match the design

</FlexItem>
</CardFooter>

<CardFooter justify="center">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Card elements are flex components, so we can justify them or align them as desired to match the designs.


> * {
max-width: 476px;
max-width: 504px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The designs have made the card bigger to compensate for the larger amount of text content.

@@ -93,9 +99,31 @@
font-weight: 400;
}

.components-popover__content {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Customizing the tooltip's popover element to get the desired display.

.woocommerce-card ~ p {
font-size: 14px;
}

.woocommerce-profile-wizard__footer {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a new footer element that holds the "skip wizard" link.

@@ -6,6 +6,8 @@
@import 'node_modules/@wordpress/base-styles/animations';
@import 'node_modules/@wordpress/base-styles/z-index';

@include wordpress-admin-schemes;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this to generate the css var that $theme-coloris replaced by

0 6px 10px rgba(0, 0, 0, 0.14);
$muriel-box-shadow-8dp: 0 5px 5px -3px rgba(0, 0, 0, 0.2), 0 8px 10px 1px rgba(0, 0, 0, 0.14),
0 3px 14px 2px rgba(0, 0, 0, 0.12);
$muriel-box-shadow-1dp: 0 2px 1px -1px rgba(0, 0, 0, 0.2),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My editor formatted these probably a prettier setting. Happy to revert this if its a pain.

@@ -53,7 +54,7 @@
$background-color: $white;
$background-color-hover: $light-gray-100;
$border-color: $light-gray-tertiary;
$foreground-color: $theme-color;
$foreground-color: var(--wp-admin-theme-color);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use the new css var

Copy link
Collaborator

Choose a reason for hiding this comment

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

$theme-color was derived from theme(button) in Gutenberg and evaluated in using postCSS. In our codebase we have variations like theme(primary) or theme(outlines). Those may need to be replaced as well.

@@ -24,7 +22,22 @@
}
}

.components-text-control__input {
// @wordpress/components styles for text control target all types, so we must also do that
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a change from the latest update to @wordpress/components

@samueljseay samueljseay changed the title Add/4566 Store details step - add contextual information and skip link Jul 2, 2020
Copy link
Collaborator

@psealock psealock left a comment

Choose a reason for hiding this comment

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

Nice job @samueljseay ! This is a nice change and clean up. I made a couple comments, but have run out of time and will give it a deeper look tomorrow. I am also getting an error when I npm start, Error: Undefined variable: "$blue-medium-focus". I think that variable needs to be removed or mapped.

I think, as mentioned below, the update to base styles and components may have more issues lurking in the codebase and I'm wondering if those updates don't warrant a separate PR. This way we can discuss and test those changes in isolation. What do you think?

</H>
<H className="woocommerce-profile-wizard__header-subtitle">
{ __(
'This will help us configure your store and get you started quickly',
"Tell us about your store and we'll get you set up in no time",
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, its wrapped in __(), so the translations are handled elsewhere.

@@ -53,7 +54,7 @@
$background-color: $white;
$background-color-hover: $light-gray-100;
$border-color: $light-gray-tertiary;
$foreground-color: $theme-color;
$foreground-color: var(--wp-admin-theme-color);
Copy link
Collaborator

Choose a reason for hiding this comment

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

$theme-color was derived from theme(button) in Gutenberg and evaluated in using postCSS. In our codebase we have variations like theme(primary) or theme(outlines). Those may need to be replaced as well.

@psealock psealock added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed [Status] Needs Review labels Jul 2, 2020
@samueljseay
Copy link
Contributor Author

@psealock thanks for the feedback. I will separate out a PR for the base-styles update. and I'll look at the other theme variations. Strange I didn't see that CSS variable error in my environment, but I'll investigate.

About the translations, sorry my question wasn't very clear, I was more wondering, is there anything we need to do to provide translations for these new strings in other languages?

@samueljseay
Copy link
Contributor Author

I'll close this for now and separate out a PR for the base-styles update first.

@samueljseay samueljseay closed this Jul 2, 2020
@psealock
Copy link
Collaborator

psealock commented Jul 3, 2020

About the translations, sorry my question wasn't very clear, I was more wondering, is there anything we need to do to provide translations for these new strings in other languages?

No we don't. Once on the .org repository, translators will translate the strings into different languages. See https://translate.wordpress.org/projects/wp-plugins/woocommerce/ for more on how thats done.

@samueljseay samueljseay deleted the add/4566 branch September 8, 2020 21:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
focus: components Issues for woocommerce components focus: onboarding needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. tool: monorepo infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store details step - add contextual information and skip link
2 participants