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.

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

Feature/country field disabling when editing Address Form #595

Closed
wants to merge 3 commits into from

Conversation

efremov-av
Copy link

@efremov-av efremov-av commented Jul 4, 2024

What is the purpose of this pull request?

For profile V2 addresses schema we can´t change the country of an existing address because the schema is different per country so we are disabling this option when editing an address to be compliant with this structure.

How should this be manually tested?

Account to test: dunnesstoresqa
Workspace: devalex

https://devalex--dunnesstoresqa.myvtex.com/account

Open Address Edit form in My Account Addresses

Screenshots or example usage

Before:

Uploading Screen Recording 2024-07-04 at 12.30.43.mov…

After:

Screen.Recording.2024-07-04.at.12.26.51.mov
Снимок экрана 2024-07-04 в 12 16 32

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Requires change to documentation, which has been updated accordingly.

@efremov-av efremov-av requested a review from a team as a code owner July 4, 2024 05:17
Copy link
Contributor

vtex-io-ci-cd bot commented Jul 4, 2024

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)

  • Minor (backwards-compatible functionality)

  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@efremov-av efremov-av changed the base branch from main to 4.x July 4, 2024 05:17
@beatrizmaselli beatrizmaselli changed the title Feature/country field disabling when edit Feature/country field disabling when editing Address Form Jul 5, 2024
@@ -50,6 +50,10 @@ class CountrySelector extends Component {
value: address.country.value,
}

if (!!address?.addressId?.value) {
address.country.disabled = true
Copy link
Contributor

@lpolon lpolon Jul 8, 2024

Choose a reason for hiding this comment

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

I believe this is not a good way to archive the desired behaviour.

As far as I can tell, even though it is a old project, I can't find any other example in the project where there is a mutation of props.

As far as I can tell, This change is a big no-no, because React assumes that props are readonly:
https://react.dev/reference/rules/components-and-hooks-must-be-pure#props
https://stackoverflow.com/questions/51435476/why-props-in-react-are-read-only

Please let me know whatever I am missing in my understanding.

Reading the project, my understanding is that all address form state should be derived from the address itself while it does not own the state.

My suggestion is to just pass this value where you need. Specifically:
https://github.com/vtex-apps/my-account/blob/8567571485d205a97e38ff1e6ec914096df4d171/react/components/pages/AddressEdit.tsx#L70
Screenshot 2024-07-08 at 12 24 34

@lpolon
Copy link
Contributor

lpolon commented Jul 8, 2024

If you are willing to merge this PR, I suggest investigating why the build step on the CI is still peding. I just clicked "rebuild" from details. Let's see.

edit: build is done

@lpolon lpolon requested a review from wagnerlduarte July 8, 2024 15:44
@beatrizmaselli
Copy link
Contributor

Closing this PR for this:
vtex-apps/my-account#303

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants