Skip to content

PROD-3109 PROD-3106 PROD-2708 PROD-2640 -> Fix issues in self service -> dev #413

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

Merged
merged 11 commits into from
Nov 16, 2022

Conversation

asadath1395
Copy link
Contributor

Fixes the following issues in self service project

  1. PROD-2640 Fix package type not selected when users comes back from the initial draft
  2. PROD-2708 Improve readability of the bug hunt package
  3. PROD-3106 Fix Bug hunt package details are not displayed in the WM challenge specification and challenge details page
  4. PROD-3109 Fix text in the 'What will I receive?' section does not match the provided text

Copy link
Contributor

@brooketopcoder brooketopcoder left a comment

Choose a reason for hiding this comment

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

@asadath1395 This looks great! Nice work.

I just have a couple super minor comments. Thanks!

margin-top: $space-xxl;
}

.body-medium-bold {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than completely redefining a global style, body-medium-bold, can we find the global style that actually matches the design? Figma designs have the name of the font we should use.

The only things we should be overriding for global fonts are related to padding/layout.

margin-bottom: $space-xxl;
}

.body-medium-bold {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here?

@@ -188,9 +215,14 @@
}

svg.card-row-icon {
height: 14px;
width: 14px;
height: 24px;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use the _icons.mixins to size icons.

display: inline;
margin-right: 6px;

@include ltemd {
height: 16px;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -77,12 +77,13 @@ const FormCardSet: React.FC<FormCardSetProps> = ({ name, cards, onChange, value
{(
<span className={cn(styles['card-row-col'], styles['center'])}>
{ row.valueIcon ?
<IconOutline.CheckIcon width={18} height={16} /> :
<IconOutline.CheckIcon width={28} height={20} /> :
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use the mixins in the stylesheets to size icons.

@@ -0,0 +1,16 @@
import { currencyFormat } from '../../../src/utils'
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a confusing naming convention, but /src-ts/utils in this case refers specifically to the utilities in the header, like Account Settings. See the README.

Anything related to bughunt and the work tool needs to be located w/in the /src-ts/tools/work directory. Perhaps, this would go in the src-ts/tools/work/work-lib/work-provider/work-functions/work.functions.ts?

Also, we should not be importing anything from the /src directory into the /src-ts directory unless we absolutely have to. And then there should be comment explaining why it has to be imported.

In this case, please use the textFormatMoneyLocaleString method from the /src-ts/lib to format money.

@asadath1395
Copy link
Contributor Author

@brooketopcoder Could you please re-review this? Thanks

Copy link
Contributor

@brooketopcoder brooketopcoder left a comment

Choose a reason for hiding this comment

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

Excellent--approved.

@brooketopcoder
Copy link
Contributor

@asadath1395 Please do not merge this until after the production deployment this afternoon.

@brooketopcoder brooketopcoder merged commit adf197f into dev Nov 16, 2022
@brooketopcoder brooketopcoder deleted the PROD-2614_Self-service_payment branch November 16, 2022 19:35
@brooketopcoder brooketopcoder restored the PROD-2614_Self-service_payment branch November 16, 2022 19:36
@brooketopcoder brooketopcoder deleted the PROD-2614_Self-service_payment branch November 18, 2022 23:15
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.

2 participants