-
Notifications
You must be signed in to change notification settings - Fork 27
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
Asub 8384/subs profile #2107
Asub 8384/subs profile #2107
Conversation
Bumps [@babel/preset-react](https://github.com/babel/babel/tree/HEAD/packages/babel-preset-react) from 7.23.3 to 7.24.1. - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.24.1/packages/babel-preset-react) --- updated-dependencies: - dependency-name: "@babel/preset-react" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Added esc event listener to useEffect --------- Co-authored-by: Malavika Koppula <mkreddy1110@gmail.com>
* Added flex to section-title links --------- Co-authored-by: Malavika Koppula <mkreddy1110@gmail.com>
Bumps [algoliasearch](https://github.com/algolia/algoliasearch-client-javascript) from 4.23.1 to 4.23.2. - [Release notes](https://github.com/algolia/algoliasearch-client-javascript/releases) - [Changelog](https://github.com/algolia/algoliasearch-client-javascript/blob/master/CHANGELOG.md) - [Commits](algolia/algoliasearch-client-javascript@4.23.1...4.23.2) --- updated-dependencies: - dependency-name: algoliasearch dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [eslint-plugin-react](https://github.com/jsx-eslint/eslint-plugin-react) from 7.33.2 to 7.34.1. - [Release notes](https://github.com/jsx-eslint/eslint-plugin-react/releases) - [Changelog](https://github.com/jsx-eslint/eslint-plugin-react/blob/v7.34.1/CHANGELOG.md) - [Commits](jsx-eslint/eslint-plugin-react@v7.33.2...v7.34.1) --- updated-dependencies: - dependency-name: eslint-plugin-react dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [glob](https://github.com/isaacs/node-glob) from 10.3.10 to 10.3.12. - [Changelog](https://github.com/isaacs/node-glob/blob/main/changelog.md) - [Commits](isaacs/node-glob@v10.3.10...v10.3.12) --- updated-dependencies: - dependency-name: glob dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* add focal point code and tests * fix eslint errors * fix formatting * fix formatting p2
* Sign In with Apple * fixing linting and tests * fixing test & linting * fixing linting errors * removing update from package.json * fixing linting errors * disable eslint warnings * fixing warnings * fixing sintax * removing keys * removing only * fixing linting errors
* added dd-service-catalog.yml * updated infrastructure
* sign up with reCaptcha * fixing linting and tests * fixing linting errors * fixing linting errors
* lokalize-translation 2.3.0 * attending feedback
* THEMES-1066: updated versions of storybook and chromatic to the latest. * THEMES-1066: fixed linting errors * THEMES-1066 adjusted configs so that storybook could build without errors. * THEMES-1066 remove addon-knobs * THEMES-1066: removing more deprecated packages. * THEMES-1066: added .babelrc * THEMES-1066: moved babel config to main.js * version bump * added alias overrides * Fixed intro page * THEMES-1066: fixed webpack/babel config * THEMES-1066: restore babel.config.js * THEMES-1066: restore babel.config.js * THEMES-1066: added styling storybook addon * THEMES-1066: updated preview to just use news.scss * THEMES-1066: fixed breaking stories and updated news.scss. * THEMES-1066: version bump for storybook * THEMES-1066: corrected whitespace * THEMES-1066: Updating option for chromatic action. * THEMES-1066: updated stylelint to hopefully fix the UI Tests check * THEMES-1066: removed jsx from stylelint action input. * THEMES-1066: added env vars to chromatic action * THEMES-1066: removing test vars from workflow file.
blocks/subscriptions-block/features/SubscriptionProfileManagement/default.jsx
Outdated
Show resolved
Hide resolved
...ions-block/features/SubscriptionProfileManagement/children/BasicSubscriptionDetail/index.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @edwardcho1231
It looks good to me, just some minor styles that we can try to fix,
For example:
- the modal
- Gap between payment method and next bill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edwardcho1231 before merge, do you mind check with Pardis, where we should provide the error message, in case cancelation/recue fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your hard work on this @edwardcho1231 , LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @edwardcho1231 by checking something for a demo I noticed something.
The info provided for inactive subscriptions should be a little bit different. However I'm seeing this. Do you mind check this
Payment method & billing address shouldn't appear. Instead of that, we need to show "Subscription period"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @edwardcho1231 for all these changes, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edwardcho1231 , left some questions and suggested cleanup on the PR. Also there's quite a few logical property conversions that are needed.
...ions-block/features/SubscriptionProfileManagement/children/BasicSubscriptionDetail/index.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes all look good, approved!
27458c9
into
arc-themes-release-version-2.3.1
Be sure to run
npm test
,npm run lint
, and write detailed test steps before requesting reviewersDescription
Jira Ticket: ASUB-8384
Information about what you changed for this PR. Include any dependencies, companion PRs, relevant screenshots, or config changes.
Test Steps
Add detailed test steps a reviewer must complete to test this PR
git checkout {branch name}
npx fusion start -f -l {blocks to link}