NTP Settings Design Fixes #6351
Conversation
| stackPosition: number | ||
| } | ||
|
|
||
| const getBackgroundRule = (position: number) => { |
There was a problem hiding this comment.
I originally wrote something to dynamically generate these gradient rules based on the position, but this is more explicit and readable
ryanml
changed the title
| @@ -137,6 +137,7 @@ interface Props { | |||
| disconnectInProgress: boolean | |||
| authInvalid: boolean | |||
| selectedView: string | |||
| stackPosition: number | |||
There was a problem hiding this comment.
nit: instead of all this repeated props, you could have something like
[another file]
type WidgetProps {
stackPosition: number
...
}
[this file]
type Props = WidgetProps & {
...binance-specific-props...
}
|
|
||
| return ( | ||
| <StyledTitleTab onClick={onShowContent}> | ||
| <StyledTitleTab onClick={onShowContent} stackPosition={stackPosition}> |
There was a problem hiding this comment.
nit (continued): and here do <StyledTitleTab ...getWidgetTitleProps(this.props)>
...but that's if you didn't want to do the repetition ;-)
| ` | ||
|
|
||
| export const SettingsFeatureBody = styled<{}, 'section'>('section')` | ||
| padding: 10px 16px 0; | ||
| height: 305px; | ||
| padding: 10px 16px 10px; |
There was a problem hiding this comment.
nit: the extra 10px is now redundant
|
Verification PASSED on As per brave/brave-browser#11103 (comment), verified that the suggestions via brave/brave-browser#10318 (comment) and #6015 (comment) have been addressed.
Also checked the following:
|
|
@ryanml @karenkliu possible issues/questions. I don't think any of these are blocking and we can continue uplifting #6372. However, let me know if I should create follow up issues. Looks like the font is still
The height of
|
|
Talked to @ryanml and @karenkliu re: the above, both are known and not issues 👍 |
Fixes: brave/brave-browser#11103
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
See issue
Reviewer Checklist:
After-merge Checklist:
changes has landed on.