Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rewards settings style #2173

Merged
merged 2 commits into from
Apr 23, 2019
Merged

Rewards settings style #2173

merged 2 commits into from
Apr 23, 2019

Conversation

rossmoody
Copy link
Contributor

@rossmoody rossmoody commented Apr 5, 2019 •
edited
Loading

Resolves: brave/brave-browser#4035
Resolves: brave/brave-browser#4171

Brave UI SHA update and a few string parity updates.

Original Brave UI PR: brave/brave-ui#442

@rossmoody rossmoody self-assigned this Apr 5, 2019
@rossmoody rossmoody mentioned this pull request Apr 5, 2019
Merged
@rossmoody rossmoody marked this pull request as ready for review April 12, 2019 00:01
@rossmoody rossmoody force-pushed the rewards-settings branch 2 times, most recently from 619d8ed to 2952a65 Compare April 12, 2019 00:17
@rossmoody
Copy link
Contributor Author

@NejcZdovc This one is ready for your review.

@jsecretan jsecretan mentioned this pull request Apr 19, 2019
Closed
@rossmoody rossmoody added this to the 0.66.x - Nightly milestone Apr 19, 2019
@jasonrsadler
Copy link
Contributor

Screen Shot 2019-04-19 at 4 29 20 PM

Text alignment for headers (at least for checkboxes) seems should be a little more to the left (based off spec photos from issue def)

@jasonrsadler
Copy link
Contributor

Screen Shot 2019-04-19 at 4 33 20 PM

Not sure if part of PR but text underlaps bullet points (If NA to this PR, or if this is what we want, ignore)

@rossmoody
Copy link
Contributor Author

@jasonrsadler Just making sure, I think that screenshot you posted might be the "before" version. After should render more like this: https://d.pr/i/k8dSg2

I think we should do a followup for the wallet empty panel. From my gatherings, it looks like those updates would be back in Brave UI and might need a little more style help. I think maybe we'd be better off doing an ordered list instead of bullets with hard breaks.

Had one remaining update that fixes the grant alerts not inheriting the width of the column.

@rossmoody rossmoody force-pushed the rewards-settings branch 2 times, most recently from 3062201 to aac0a8c Compare April 19, 2019 22:33
@jasonrsadler
Copy link
Contributor

Pulling and rebuilding and I'll double check.

👍 on wallet panel.

@jasonrsadler
Copy link
Contributor

jasonrsadler commented Apr 19, 2019 •
edited
Loading

Yep. This is rewards-settings branch

Yep. Didn't run npm i on brave-ui. Checks out 👍

@jasonrsadler
Copy link
Contributor

Screen Shot 2019-04-20 at 1 25 07 PM

👍

jasonrsadler
jasonrsadler previously approved these changes Apr 20, 2019
View reviewed changes
@NejcZdovc
Copy link
Contributor

@rossmoody could you please check if grant notifications are the same. Text in ugp looks smaller then in ads grant

@rossmoody
Copy link
Contributor Author

Good to go! This is them overlapping eachother.

jpg

@jasonrsadler jasonrsadler self-requested a review April 22, 2019 17:52
@rossmoody
rewards settings style
d5cdb53 
package.json update
@rossmoody
grant alert width fix
9ba457c 
Update settingsPage.tsx
@rossmoody rossmoody merged commit b0e01ff into master Apr 23, 2019
@rossmoody rossmoody deleted the rewards-settings branch April 23, 2019 19:33
@karenkliu
Copy link

Can we use "Your earnings from ads are available." instead of "Your Ads earnings are available." so we match the grant messaging in the Your Wallet panel?

@rossmoody
Copy link
Contributor Author

Good spot. This PR merged already but if you open an issue for this change I could take it on.

@karenkliu
Copy link

Should I open in brave-browser?

@rossmoody
Copy link
Contributor Author

Yeah, Brave Browser issues would do the trick.

@kjozwiak kjozwiak mentioned this pull request May 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonrsadler jasonrsadler jasonrsadler approved these changes

@ryanml ryanml Awaiting requested review from ryanml

@NejcZdovc NejcZdovc Awaiting requested review from NejcZdovc

Assignees

@rossmoody rossmoody

Labels
feature/rewards
Projects
None yet
Milestone
0.66.x - Release
Development

Successfully merging this pull request may close these issues.

Claim banners are of different text size Polish Rewards settings panels
5 participants
@rossmoody @jasonrsadler @NejcZdovc @karenkliu @bsclifton