-
Notifications
You must be signed in to change notification settings - Fork 41
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
Feature/brave rewards ui rebase2 #1162
Feature/brave rewards ui rebase2 #1162
Conversation
92c8cec
to
5444f24
Compare
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.
Looking pretty good so far. A few comments besides the inlines in no particular order:
-
We should add
BRAVE_REWARDS_EMAIL_WHITELIST
to base.yml so it can be used with docker -
I think we should add at least add tests for the Publishers::SiteBannersController as we discussed
-
The "Instant Donation" modal does not exactly meet the spec in terms of spacing, font size, and buttons.
Spec:
This Pr: -
When you click "Instant Donation" then "Preview Banner" then "Close" then "Instant Donation" again, the modal appears offset to the left
-
Hovering over the photo icons do not produce the pointer cursor that you'd expect
-
It's not intuitive how to edit the donation amounts
-
The 5 BAT donation amount is pre highlighted, though it has no significance to the publisher which I found confusing
-
Donation amounts do not update until after the user clicks "Save Change" AND refreshes the page
-
There is no way to go from "preview" to "edit", but there is the other way around.
content_type = "image/png" | ||
extension = ".png" | ||
else | ||
# TODO: Throw an exception here |
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.
Should we resolve this TODO?
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.
Going to track in https://github.com/brave-intl/publishers/issues/1179
app/models/site_banner.rb
Outdated
elsif Rails.env.staging? | ||
return "https://rewards-stg.s3.us-east-2.amazonaws.com/#{object.blob.key}" | ||
elsif Rails.env.production? | ||
# TODO |
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.
Should we resolve this TODO?
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.
Yes I'll resolve. as part of this PR
verified_channels.find_each do |verified_channel| | ||
if @excluded_channel_ids.include?(verified_channel.details.channel_identifier) | ||
excluded = true | ||
@excluded_verified_channel_ids.push(verified_channel.details.channel_identifier) | ||
else | ||
excluded = false | ||
end | ||
channels.push([verified_channel.details.channel_identifier, true, excluded]) | ||
channels.push([verified_channel.details.channel_identifier, true, excluded, verified_channel.publisher&.site_banner&.read_only_react_property || {}]) |
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.
should we check to make sure this change doesn't break how the browser parses the response?
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 confirmed with ryanml that this won't break anything
@@ -1,5 +1,5 @@ | |||
# Builds a list of distinct channels that are either verified, excluded or both for the Brave Browser. | |||
# | |||
# | |||
# Each channel is an array: | |||
# [channel_identifier, verified, excluded] |
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.
We should update this comment with the new response format
Closes #1092
31502bd
to
9dc5778
Compare
publisher_id: current_publisher.id, | ||
title: params[:title], | ||
donation_amounts: donation_amounts, | ||
default_donation: donation_amounts[1], |
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.
We could use some syntactic sugar and say something like donation_amounts.second
. Up to you if you wanna change it
donation_amounts = JSON.parse(params[:donation_amounts]) | ||
site_banner.update( | ||
publisher_id: current_publisher.id, | ||
title: params[:title], |
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.
Do we verify these params anywhere?
Channel.youtube_channels, | ||
Channel.twitch_channels, | ||
Channel.twitter_channels].each do |verified_channels| | ||
[Channel.left_joins(publisher: :site_banner).verified.site_channels, Channel.left_joins(publisher: :site_banner).youtube_channels, Channel.left_joins(publisher: :site_banner).twitch_channels, Channel.left_joins(publisher: :site_banner).twitter_channels].each do |verified_channels| |
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.
This is an awfully long line, and we call left_joins
on this a lot. Maybe it would make sense to introduce a scope on the Channel
model?
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.
We'll fix this later.
app/models/site_banner.rb
Outdated
if Rails.env.development? || Rails.env.test? | ||
"https://0.0.0.0:3000" + rails_blob_path(object, only_path: true) + extension | ||
elsif Rails.env.staging? | ||
return "https://rewards-stg.s3.us-east-2.amazonaws.com/#{object.blob.key}" |
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'm not a huge fan of having hard coded urls in a model instead of a configuration file. What are your thoughts on the matter?
.col-md-4 | ||
= link_to("Preview Banner", "#", class: 'btn btn-primary', id: "preview-banner-button") | ||
.col-md-4 | ||
= link_to("Customize", "#", class: 'btn btn-primary', id: "edit-banner-button") |
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.
The rest of the file is internationalized, should we do the same here?
@@ -1,3 +1,5 @@ | |||
const { environment } = require('@rails/webpacker') | |||
const typescript = require('./loaders/typescript') |
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.
wow! it's cool to see code I contributed to webpacker actually being used!!
* Fix spacing between buttons * Use translation strings instead of hardcoding * Remove _instant_donation_button.html.slim
Add BRAVE_REWARDS_EMAIL_WHITELIST to base.yml for docker users
…on-modal Fix instant donation modal
|
||
private | ||
|
||
def update_image(attachment) |
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.
is there an attachment size limit anywhere? otherwise someone can try to take up all our s3 space
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.
Addressed in f8dc6b9c53d10642367ddfddb6c199fd0b3723a6
. Test is in publishers/test/controllers/publishers/site_banners_controller.rb
|
||
handleSubmit(event) { | ||
const url = '/publishers/' + this.props.publisher_id + "/site_banners"; | ||
var request = new XMLHttpRequest(); |
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.
this appears to be unused? (you can add a js lint command in travis to catch these things)
package.json
Outdated
"@types/react": "^16.4.7", | ||
"@types/react-dom": "^16.0.6", | ||
"babel-preset-react": "^6.24.1", | ||
"brave-ui": "0.23.0", |
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.
is there any reason to pin this to a specific version?
package.json
Outdated
"react": "^16.4.1", | ||
"react-dom": "^16.4.1", | ||
"styled-components": "^3.2.6", | ||
"ts-loader": "3.5.0", |
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.
is there any reason to pin this to a specific version?
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.
Oh hey! I actually know the answer to this, because webpacker doesn't support webpack 4 in production, and they introduced this commit to lock it down
rails/webpacker@af471dc#diff-63cad75e7b69369d47bacd65341a3bf8
@yachtcaptain23 can you make sure that you lock this down?
app/models/site_banner.rb
Outdated
if Rails.env.development? || Rails.env.test? | ||
"https://0.0.0.0:3000" + rails_blob_path(object, only_path: true) + extension | ||
elsif Rails.env.staging? | ||
return "https://rewards-stg.s3.us-east-2.amazonaws.com/#{object.blob.key}" |
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.
what are the permissions on this bucket?
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.
Exposed in the URL as part of a public read only. The write permissions are stored in heroku's environment variables and configured in storage.yml
Site banner overhaul rebase
Restores modal to original size
Use uniform file size
First we calculate how much padding to add by creating a Tempfile with 1 character padding and checking the delta between the Tempfile's size and the target file size. Then we apply padding to the original file.
Feature/resize images
Fixes modal expansion bug
…size Fix file size restriction and logo image restrictions
Submitter Checklist:
Test Plan:
Reviewer Checklist:
Tests
Security:
html_safe
andraw
; escape untrusted content from users and 3rd party APIs (Rails XSS guide and OWASP XSS Prevention Cheat Sheet)