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

add qwant support #654

Merged
merged 2 commits into from
Oct 17, 2018
Merged

add qwant support #654

merged 2 commits into from
Oct 17, 2018

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Oct 16, 2018 •
edited by petemill
Loading

Fix brave/brave-browser#1607

(adds qwant to new tab page for private mode in certain locales)

@cezaraugusto cezaraugusto self-assigned this Oct 16, 2018
@cezaraugusto
Copy link
Contributor Author

language check based on ISO-639-1. ref https://en.wikipedia.org/wiki/List_of_ISO_639-1_codes

petemill
petemill approved these changes Oct 16, 2018
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic looks 👍

components/brave_new_tab_ui/components/privateTab/index.tsx Outdated
@@ -18,12 +20,23 @@ interface Props {
}

export default class NewPrivateTab extends React.PureComponent<Props, {}> {
get isQwant () {
return navigator.language === 'de' || navigator.language === 'fr'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This can be a static const defined at the module level, since it does not rely on any props or anything to do with the component

components/brave_new_tab_ui/components/privateTab/index.tsx
@@ -35,7 +48,7 @@ export default class NewPrivateTab extends React.PureComponent<Props, {}> {
render () {
const { isTor } = this.props
return (
<Page isPrivate={!isTor}>
<Page isPrivate={!isTor && !this.isQwant}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not private if it's de or fr language?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just to change the background color. I should have written isDefaultPrivate but need to level up my naming game :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cezaraugusto that's ok, when things calm down we should look to improve some naming all around the webui's

@petemill
Copy link
Member

@cezaraugusto Is there a brave-browser issue with a milestone this can go against?

@tildelowengrimm
Copy link
Contributor

tildelowengrimm commented Oct 16, 2018 •
edited
Loading

I think brave/brave-browser#1607 is the issue in question.

@petemill
Copy link
Member

Just building for testing before merging since we're on RC

@bsclifton bsclifton mentioned this pull request Oct 16, 2018
Merged
18 tasks
petemill
petemill requested changes Oct 16, 2018
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not work for me. I still saw DDG in FR context. Note that for me, when I changed language, my navigator.language was 'fr-FR'. I suspect we need to parse this a bit smarter, or use some other property.

@bsclifton bsclifton mentioned this pull request Oct 16, 2018
Closed
@petemill
Copy link
Member

Follow up to make qwant pref detection more robust brave/brave-browser#1632

petemill
petemill approved these changes Oct 16, 2018
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed language detection via startsWith

bsclifton
bsclifton approved these changes Oct 17, 2018
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely would have been nice to do this properly in C++... but I think the scope of that change is too large. We'll have to talk with @mkarolin (details captured in brave/brave-browser#1632)

Tested this PR as-is though and it does work as expected. I tested on macOS using French 😄

@petemill petemill merged commit cfe8145 into master Oct 17, 2018
@petemill petemill deleted the qwant branch October 17, 2018 00:11
petemill added a commit that referenced this pull request Oct 17, 2018
@petemill
Merge pull request #654 from brave/qwant
f18f807 
add qwant support
petemill added a commit that referenced this pull request Oct 17, 2018
@petemill
Merge pull request #654 from brave/qwant
42dc457 
add qwant support
petemill added a commit that referenced this pull request Oct 17, 2018
@petemill
Merge pull request #654 from brave/qwant
4de344f 
add qwant support
@petemill
Copy link
Member

0.57.x f18f807
0.56.x 42dc457
0.55.x 4de344f

@bbondy bbondy removed the 0.55.x label Jan 14, 2019
@bbondy bbondy added this to the 0.55.x - Release milestone Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petemill petemill petemill approved these changes

@bsclifton bsclifton bsclifton approved these changes

@tildelowengrimm tildelowengrimm Awaiting requested review from tildelowengrimm

@NejcZdovc NejcZdovc Awaiting requested review from NejcZdovc

@riastradh-brave riastradh-brave Awaiting requested review from riastradh-brave

Assignees

@cezaraugusto cezaraugusto

Labels
None yet
Projects
None yet
Milestone
0.55.x - Release
Development

Successfully merging this pull request may close these issues.
5 participants
@cezaraugusto @petemill @tildelowengrimm @bsclifton @bbondy