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

fix: add getOverrideProps to StorageManager #1020

Merged
merged 3 commits into from
Jun 6, 2023

Conversation

bombguy
Copy link
Contributor

@bombguy bombguy commented Jun 6, 2023

Problem

The override props are not being applied to StorageManager

Solution

render spread attribute for getOverrideProps

Links

Ticket

GitHub issue : aws-amplify/amplify-studio#970

Automated tests

  • Verified snapshot

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@bombguy bombguy requested a review from ErikCH June 6, 2023 20:03
@bombguy bombguy marked this pull request as ready for review June 6, 2023 20:03
@bombguy bombguy requested a review from a team as a code owner June 6, 2023 20:03
Jshhhh
Jshhhh previously approved these changes Jun 6, 2023
rtpascual
rtpascual previously approved these changes Jun 6, 2023
awinberg-aws
awinberg-aws previously approved these changes Jun 6, 2023
ErikCH
ErikCH previously approved these changes Jun 6, 2023
@reesscot
Copy link

reesscot commented Jun 6, 2023

Shouldn't it also be possible to override the wrapping Field component? I don't see an getOverrideProps call here:

      <Field
        errorMessage={errors.imgKeys?.errorMessage}
        hasError={errors.imgKeys?.hasError}
        label={\\"Images\\"}
        isRequired={false}
        isReadOnly={false}
      >

@bombguy
Copy link
Contributor Author

bombguy commented Jun 6, 2023

Shouldn't it also be possible to override the wrapping Field component? I don't see an overrides call here:

      <Field
        errorMessage={errors.imgKeys?.errorMessage}
        hasError={errors.imgKeys?.hasError}
        label={\\"Images\\"}
        isRequired={false}
        isReadOnly={false}
      >

Yes, that is also possible as well. Should the overrideProps be spread in both Field & StorageManager?

@bombguy bombguy dismissed stale reviews from ErikCH, awinberg-aws, rtpascual, and Jshhhh via 6393718 June 6, 2023 20:42
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2023

Codecov Report

Merging #1020 (12c9020) into main (1fbf44d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1020   +/-   ##
=======================================
  Coverage   93.71%   93.72%           
=======================================
  Files         122      122           
  Lines        5428     5430    +2     
  Branches     1620     1620           
=======================================
+ Hits         5087     5089    +2     
  Misses        323      323           
  Partials       18       18           
Impacted Files Coverage Δ
...s/codegen-ui-react/lib/react-component-renderer.ts 100.00% <100.00%> (ø)
...i-react/lib/utils/forms/storage-field-component.ts 96.61% <100.00%> (+0.05%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fbf44d...12c9020. Read the comment docs.

lyonsbp
lyonsbp previously approved these changes Jun 6, 2023
@Jshhhh
Copy link
Contributor

Jshhhh commented Jun 6, 2023

Shouldn't it also be possible to override the wrapping Field component? I don't see an overrides call here:

      <Field
        errorMessage={errors.imgKeys?.errorMessage}
        hasError={errors.imgKeys?.hasError}
        label={\\"Images\\"}
        isRequired={false}
        isReadOnly={false}
      >

Yes, that is also possible as well. Should the overrideProps be spread in both Field & StorageManager?

Shouldn't it also be possible to override the wrapping Field component? I don't see an overrides call here:

      <Field
        errorMessage={errors.imgKeys?.errorMessage}
        hasError={errors.imgKeys?.hasError}
        label={\\"Images\\"}
        isRequired={false}
        isReadOnly={false}
      >

Yes, that is also possible as well. Should the overrideProps be spread in both Field & StorageManager?

I think the overrideProps should be one or the other, or have a different key passed into getOverrideProps

@bombguy
Copy link
Contributor Author

bombguy commented Jun 6, 2023

The override props should only be applied to the StorageManager since the configuration is specific to the StorageManager, not Field component.

This pattern is used in other wrapper component we render. (e.g. ArrayField)

@bombguy bombguy merged commit 8ff282f into main Jun 6, 2023
@bombguy bombguy deleted the cshin/override-props-storagemanager branch June 6, 2023 21:48
bombguy added a commit that referenced this pull request Jun 19, 2023
* fix: parse operand value when field is number type (#1013)

Co-authored-by: Justin Shih <[email protected]>

* chore(release): v2.14.0 (#1014)

Co-authored-by: Justin Shih <[email protected]>

* fix: fix storage field prop import to storagemanager (#1016)

Co-authored-by: Justin Shih <[email protected]>

* chore(release): v2.14.1 (#1017)

Co-authored-by: Justin Shih <[email protected]>

* fix: add getOverrideProps to StorageManager (#1020)

* fix: add getOverrideProps to StorageManager

* fix: add overrideProps to Field component

* Revert "fix: add overrideProps to Field component"

This reverts commit 6393718.

* chore(release): v2.14.2 (#1022)

* fix: return theme declaration file from render func (#1023)

Co-authored-by: Doesnt Matter <[email protected]>

* fix: alias colliding imported model name in hasMany relationship (#1019)

Co-authored-by: Justin Shih <[email protected]>

* chore: update cli check

---------

Co-authored-by: Justin Shih <[email protected]>
Co-authored-by: Justin Shih <[email protected]>
Co-authored-by: Charles Shin <[email protected]>
Co-authored-by: David Lopez <[email protected]>
Co-authored-by: Doesnt Matter <[email protected]>
Co-authored-by: Roshane Pascual <[email protected]>
rtpascual added a commit that referenced this pull request Jun 23, 2023
* fix: parse operand value when field is number type (#1013)

Co-authored-by: Justin Shih <[email protected]>

* chore(release): v2.14.0 (#1014)

Co-authored-by: Justin Shih <[email protected]>

* fix: fix storage field prop import to storagemanager (#1016)

Co-authored-by: Justin Shih <[email protected]>

* chore(release): v2.14.1 (#1017)

Co-authored-by: Justin Shih <[email protected]>

* fix: add getOverrideProps to StorageManager (#1020)

* fix: add getOverrideProps to StorageManager

* fix: add overrideProps to Field component

* Revert "fix: add overrideProps to Field component"

This reverts commit 6393718.

* chore(release): v2.14.2 (#1022)

* fix: return theme declaration file from render func (#1023)

Co-authored-by: Doesnt Matter <[email protected]>

* fix: alias colliding imported model name in hasMany relationship (#1019)

Co-authored-by: Justin Shih <[email protected]>

* chore: update cli check

* chore: add specific cypress version to fix 12.15.0 bug (#1028)

---------

Co-authored-by: Justin Shih <[email protected]>
Co-authored-by: Justin Shih <[email protected]>
Co-authored-by: Charles Shin <[email protected]>
Co-authored-by: David Lopez <[email protected]>
Co-authored-by: Doesnt Matter <[email protected]>
letsbelopez added a commit that referenced this pull request Jul 19, 2023
* fix: parse operand value when field is number type (#1013)

Co-authored-by: Justin Shih <[email protected]>

* chore(release): v2.14.0 (#1014)

Co-authored-by: Justin Shih <[email protected]>

* fix: fix storage field prop import to storagemanager (#1016)

Co-authored-by: Justin Shih <[email protected]>

* chore(release): v2.14.1 (#1017)

Co-authored-by: Justin Shih <[email protected]>

* fix: add getOverrideProps to StorageManager (#1020)

* fix: add getOverrideProps to StorageManager

* fix: add overrideProps to Field component

* Revert "fix: add overrideProps to Field component"

This reverts commit 6393718.

* chore(release): v2.14.2 (#1022)

* fix: return theme declaration file from render func (#1023)

Co-authored-by: Doesnt Matter <[email protected]>

* fix: alias colliding imported model name in hasMany relationship (#1019)

Co-authored-by: Justin Shih <[email protected]>

* chore: update cli check

---------

Co-authored-by: Justin Shih <[email protected]>
Co-authored-by: Justin Shih <[email protected]>
Co-authored-by: Charles Shin <[email protected]>
Co-authored-by: David Lopez <[email protected]>
Co-authored-by: Doesnt Matter <[email protected]>
Co-authored-by: Roshane Pascual <[email protected]>
letsbelopez added a commit that referenced this pull request Jul 19, 2023
* feat: add graphql support for components

* feat: add graphql support for create form (#1008)

* feat: add graphql support for create form with relationships (#1010)

* Update GraphQL API import and refactor GraphQL call expressions (#1012)

* chore: update graphql api import

* chore: refactor graphql call expressions to common function

* chore: use pluralize for graphql list queries

* feat: add graphql support for update form (#1015)

* feat: add graphql support for update form

* fix: update snapshot

* feat: add more assertions to check for graphql support

* fix: removing obselete snapshot

* fix: force update mutation to use id var

* chore: add declaration snapshots (#1018)

* Update from main (#1025)

* fix: parse operand value when field is number type (#1013)

Co-authored-by: Justin Shih <[email protected]>

* chore(release): v2.14.0 (#1014)

Co-authored-by: Justin Shih <[email protected]>

* fix: fix storage field prop import to storagemanager (#1016)

Co-authored-by: Justin Shih <[email protected]>

* chore(release): v2.14.1 (#1017)

Co-authored-by: Justin Shih <[email protected]>

* fix: add getOverrideProps to StorageManager (#1020)

* fix: add getOverrideProps to StorageManager

* fix: add overrideProps to Field component

* Revert "fix: add overrideProps to Field component"

This reverts commit 6393718.

* chore(release): v2.14.2 (#1022)

* fix: return theme declaration file from render func (#1023)

Co-authored-by: Doesnt Matter <[email protected]>

* fix: alias colliding imported model name in hasMany relationship (#1019)

Co-authored-by: Justin Shih <[email protected]>

* chore: update cli check

---------

Co-authored-by: Justin Shih <[email protected]>
Co-authored-by: Justin Shih <[email protected]>
Co-authored-by: Charles Shin <[email protected]>
Co-authored-by: David Lopez <[email protected]>
Co-authored-by: Doesnt Matter <[email protected]>
Co-authored-by: Roshane Pascual <[email protected]>

* feat: update GraphqlRenderConfig to use separate props for each file path

* feat: graphql support for collections (#1027)

* feat: graphql support for collections

* chore: refactor collection element to helper function and update addMappedImport

* feat: adding graphql support for update forms with relationship (#1030)

* fix: wrapping list graphql result for await (#1032)

* feat: add graphql support for relationship forms with autocomplete field

* fix: autocomplete options prop using incorrect records (#1035)

* feat: add graphql support for relationship forms with autocomplete field

* fix: autocomplete options prop using incorrect records

* feat: add helpers to identify api dependencies

* fix: use related model name instead of field in generating graphql qu… (#1039)

* fix: use related model name instead of field in generating graphql queries

* fix: updating invalid schema in integ test

* feat: exit early in integ test failure

* fix: lowering nyc coverage threshold

---------

Co-authored-by: Albert Winberg <[email protected]>
Co-authored-by: Roshane Pascual <[email protected]>
Co-authored-by: Charles Shin <[email protected]>
Co-authored-by: awinberg-aws <[email protected]>
Co-authored-by: Justin Shih <[email protected]>
Co-authored-by: Justin Shih <[email protected]>
Co-authored-by: David Lopez <[email protected]>
Co-authored-by: Roshane Pascual <[email protected]>
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.

8 participants