-
Notifications
You must be signed in to change notification settings - Fork 14
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
Conversation
…s not match the provided text
…the WM challenge specification and challenge details page
…k from the initial draft
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.
@asadath1395 This looks great! Nice work.
I just have a couple super minor comments. Thanks!
margin-top: $space-xxl; | ||
} | ||
|
||
.body-medium-bold { |
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.
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 { |
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.
same here?
@@ -188,9 +215,14 @@ | |||
} | |||
|
|||
svg.card-row-icon { | |||
height: 14px; | |||
width: 14px; | |||
height: 24px; |
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.
let's use the _icons.mixins
to size icons.
display: inline; | ||
margin-right: 6px; | ||
|
||
@include ltemd { | ||
height: 16px; |
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.
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} /> : |
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.
let's use the mixins in the stylesheets to size icons.
src-ts/utils/bug-hunt/index.ts
Outdated
@@ -0,0 +1,16 @@ | |||
import { currencyFormat } from '../../../src/utils' |
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.
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.
@brooketopcoder Could you please re-review this? Thanks |
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.
Excellent--approved.
@asadath1395 Please do not merge this until after the production deployment this afternoon. |
Fixes the following issues in self service project