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

Numeric attributes #1065

Merged
merged 14 commits into from
Apr 29, 2021
Merged

Numeric attributes #1065

merged 14 commits into from
Apr 29, 2021

Conversation

piotrgrundas
Copy link
Contributor

@piotrgrundas piotrgrundas commented Apr 15, 2021 •
edited
Loading

I want to merge this change because of Introducing numeric attributes
#6790

PR intended to be tested with API branch: feature/numeric-attributes

Screenshots

image
image
image
image

Pull Request Checklist

  1. This code contains UI changes
  2. All visible strings are translated with proper context including data-formatting
  3. Attributes [data-test-id] are added for new elements
  4. Changes are mentioned in the changelog
  5. The changes are tested in different browsers and in light/dark mode

Test environment config

API_URI=https://qa.staging.saleor.cloud/graphql/

@piotrgrundas piotrgrundas marked this pull request as ready for review April 15, 2021 22:40
@piotrgrundas piotrgrundas requested review from a team, orzechdev and bmigirl and removed request for a team April 16, 2021 07:06
@piotrgrundas piotrgrundas requested a review from pwgryglak April 16, 2021 07:12
pwgryglak
pwgryglak suggested changes Apr 16, 2021
View reviewed changes
Copy link

@pwgryglak pwgryglak left a comment

Choose a reason for hiding this comment

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

Unit is not shown on the product details. Need to change that

@CLAassistant
Copy link

CLAassistant commented Apr 16, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@piotrgrundas piotrgrundas requested a review from pwgryglak April 18, 2021 07:22
@piotrgrundas piotrgrundas requested a review from jwm0 April 18, 2021 07:45
piotrgrundas
piotrgrundas commented Apr 19, 2021
View reviewed changes
src/attributes/components/AttributeDetails/messages.tsx
lbs: "lbs",
squareFt: <>ft&sup2;</>,
squareYd: <>yd&sup2;</>,
squareInch: <>in&sup2;</>
Copy link
Contributor Author

@piotrgrundas piotrgrundas Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I belive we don't need to translate almost all of them,

@eCzerniak eCzerniak self-requested a review April 19, 2021 07:34
@eCzerniak eCzerniak removed their request for review April 19, 2021 08:05
@piotrgrundas piotrgrundas mentioned this pull request Apr 19, 2021
Merged
11 tasks
@piotrgrundas piotrgrundas requested a review from orzechdev April 19, 2021 09:06
bmigirl
bmigirl suggested changes Apr 19, 2021
View reviewed changes
schema.graphql Show resolved Hide resolved
src/attributes/components/AttributeDetails/NumericUnits.tsx Outdated Show resolved Hide resolved
src/attributes/components/AttributeDetails/NumericUnits.tsx Show resolved Hide resolved
src/attributes/components/AttributeDetails/NumericUnits.tsx Outdated Show resolved Hide resolved
src/attributes/components/AttributeDetails/utils.ts Outdated Show resolved Hide resolved
src/attributes/components/AttributeDetails/messages.tsx Outdated Show resolved Hide resolved
src/products/components/ProductExportDialog/ProductExportDialogInfo.tsx Outdated Show resolved Hide resolved
src/products/components/ProductExportDialog/messages.ts Outdated Show resolved Hide resolved
src/attributes/components/AttributeDetails/NumericUnits.tsx Outdated Show resolved Hide resolved
jwm0
jwm0 suggested changes Apr 19, 2021
View reviewed changes
schema.graphql Outdated Show resolved Hide resolved
src/attributes/components/AttributeDetails/NumericUnits.tsx Outdated Show resolved Hide resolved
src/attributes/components/AttributeDetails/NumericUnits.tsx Outdated Show resolved Hide resolved
src/attributes/components/AttributeDetails/messages.tsx Outdated Show resolved Hide resolved
src/attributes/components/AttributeDetails/utils.ts Outdated Show resolved Hide resolved
src/attributes/components/AttributePage/AttributePage.tsx Outdated Show resolved Hide resolved
src/hooks/useForm.ts Outdated Show resolved Hide resolved
@piotrgrundas piotrgrundas self-assigned this Apr 19, 2021
@eCzerniak eCzerniak self-requested a review April 20, 2021 07:06
@piotrgrundas piotrgrundas requested a review from jwm0 April 20, 2021 09:51
@eCzerniak
Copy link
Contributor

Issues:

  1. When user tries to add new Attribute with InputType: Imperial -> Volume -> clicking on unit field causes and error.
  2. User can use numeric attribute as variant attribute but it should be not possible.
  3. Selected attribute units are not visible on the product.

All topics have been discussed on slack.

@piotrgrundas piotrgrundas force-pushed the numeric-attributes branch 3 times, most recently from 95a6bc8 to 6ed4c52 Compare April 20, 2021 21:42
@jwm0 jwm0 merged commit 2a21609 into saleor:master Apr 29, 2021
@bmigirl bmigirl mentioned this pull request Jun 8, 2021
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pwgryglak pwgryglak pwgryglak approved these changes

@eCzerniak eCzerniak eCzerniak approved these changes

@jwm0 jwm0 jwm0 approved these changes

@orzechdev orzechdev orzechdev approved these changes

@bmigirl bmigirl Awaiting requested review from bmigirl

Assignees

@piotrgrundas piotrgrundas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@piotrgrundas @CLAassistant @eCzerniak @orzechdev @bmigirl @jwm0 @pwgryglak