-
Notifications
You must be signed in to change notification settings - Fork 940
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
Cross promotional modal dp #6171
Conversation
Brave works everywhere | ||
</message> | ||
<message name="IDS_DOWNLOAD_BRAVE" desc="Cross promotional modal text"> | ||
Download Brave at brave.com on your laptop ot tablet for a faster and more private web |
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.
laptop or tablet
?
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.
oops sorry. it's a typo. i will update it
Download Brave at brave.com on your laptop ot tablet for a faster and more private web | ||
</message> | ||
<message name="IDS_BRAVE_URL" desc="Brave url"> | ||
brave.com |
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.
Not sure if we need it as string here as it doesn't require translation
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 agree. i will remove it from strings and add it in the class as constant
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.
Where do we use it?
ss.setSpan(foregroundSpan, indexOfSpan, | ||
(indexOfSpan + getResources().getString(R.string.brave_url).length()), Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); | ||
mCrossPromoText.setText(ss); | ||
|
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.
nit: excessive line
params.height = LinearLayout.LayoutParams.WRAP_CONTENT; | ||
getDialog().getWindow().setAttributes(params); | ||
} | ||
} |
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.
nit: in chromium they add empty line at the end of java files, I think we should do it too for consistency
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.
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.
++
Resolves : brave/brave-browser#10571
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Follow the test plan on the issue and follow https://www.figma.com/file/atoL4LN2kd8ILnPGwohUoV/Mobile-onboarding-v2?node-id=697%3A5848 for the design spec
Reviewer Checklist:
After-merge Checklist:
changes has landed on.