Skip to content
This repository has been archived by the owner on May 22, 2021. It is now read-only.

Restricted the banner from showing on unsupported browsers #702

Merged
merged 1 commit into from Jan 13, 2018
Merged

Restricted the banner from showing on unsupported browsers #702

merged 1 commit into from Jan 13, 2018

Conversation

himanish-star
Copy link
Contributor

@himanish-star himanish-star commented Jan 12, 2018 •
edited

fixes : #663
Here is a GIF of the changes
ezgif com-video-to-gif 11

dannycoates
dannycoates suggested changes Jan 12, 2018
View reviewed changes
app/routes/index.js Outdated
@@ -9,6 +9,9 @@ const app = choo();

function body(template) {
return function(state, emit) {
if (state.route.startsWith('/unsupported/')) {
state.promo = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will toggle the banner off for the remainder of the session, which for almost all cases is still fine because folks who end up here aren't likely to go to any other routes, but we can be more precise, so we should be.

I suggest creating a helper function called showBanner (or something) that combines your comparison here with stuff on line 16 below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sure I'll do this

@himanish-star
Copy link
Contributor Author

This will toggle the banner off for the remainder of the session

You mean that state.promo would get set as false throughout the session ? And that would cause the problem.

I have made the changes but I'm afraid that I didn't quite achieve the changes you were expecting from me. I think I couldn't get the functionality right.

@himanish-star
Copy link
Contributor Author

This will toggle the banner off for the remainder of the session

After going to route /unsupported/:reason the banner is hidden but then visiting to other routes should also hide them according to you but I can't see anything like this.
ezgif com-video-to-gif 12

@dannycoates
Copy link
Contributor

After going to route /unsupported/:reason the banner is hidden but then visiting to other routes should also hide them according to you but I can't see anything like this.

I didn't explain this very well. The only page that should hide the banner is the unsupported page, all others should show it. The way you have it now is correct.

dannycoates
dannycoates suggested changes Jan 12, 2018
View reviewed changes
app/routes/index.js Outdated
@@ -7,10 +7,14 @@ const fxPromo = require('../templates/fxPromo');

const app = choo();

function showBanner(route) {
return !route.startsWith('/unsupported/');
Copy link
Contributor

Choose a reason for hiding this comment

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

we still want to check state.promo so state.promo &&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we still want to check state.promo.

Sorry, I missed upon that in a hurry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the changes

dannycoates
dannycoates suggested changes Jan 13, 2018
View reviewed changes
app/routes/index.js Outdated
@@ -7,10 +7,14 @@ const fxPromo = require('../templates/fxPromo');

const app = choo();

function showBanner(state) {
return !(state.promo && state.route.startsWith('/unsupported/'));
Copy link
Contributor

Choose a reason for hiding this comment

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

The parameters look good, but the boolean logic isn't quite what we want. For example this return true when both are false.

The truth table we want is:

show promo unsupported
true true false
false true true
false false true
false false false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the changes.

@himanish-star
Copy link
Contributor Author

himanish-star commented Jan 13, 2018 •
edited

Sorry for troubling you @dannycoates , but I have a few questions

The only page that should hide the banner is the unsupported page, all others should show it.

I also meant the same. What I was trying to convey was that when I had pushed a commit for the first time where I had used the logic state.promo = false , you said this

This will toggle the banner off for the remainder of the session

But what I want to say is that (I hade also made a GIF above) besides using state.promo = false (which I know is logically incorrect ) the banner was behaving the way it was meant to behave i.e. hidden in case of unsupported route and visible in case of other routes. I guess the reason behind this is that the state.promo is always reset to either 'grey' or 'blue'. So it doesn't make any difference if state.promo was set to false or not.
EDIT : Was my first commit reviewed for changes because of the fact that state.promo = false would set state.promo as false for the entire session ?
Also as I was going through some issues I saw how send was working in edge. But whenever I open send in edge no matter which ever route I go to I always land up on /unsupported/edge

@shikhar-scs
Copy link
Contributor

Also as I was going through some issues I saw how send was working in edge. But whenever I open send in edge no matter which ever route I go to I always land up on /unsupported/edge

Might be because you are working in chrome.

@himanish-star
Copy link
Contributor Author

@shikhar-scs , read clearly what I have commented

@shikhar-scs
Copy link
Contributor

Yep my bad.

@dannycoates
Copy link
Contributor

banner was behaving the way it was meant to behave i.e. hidden in case of unsupported route and visible in case of other routes

It was in some cases but not others. If you were using the address bar to go to new urls, promo would get reset. If you were navigating from links on the page or the back button, promo would not get reset. Your first PR did not work as it should in the second case.

So it doesn't make any difference if state.promo was set to false or not.

It does make a difference. In general its a bad idea to change state when unnecessary. There may be other parts of the system that are affected by it.

Was my first commit reviewed for changes because of the fact that state.promo = false would set state.promo as false for the entire session ?

Yes

@dannycoates dannycoates merged commit f3d77fd into mozilla:master Jan 13, 2018
@himanish-star
Copy link
Contributor Author

himanish-star commented Jan 14, 2018 •
edited

If you were navigating from links on the page or the back button, promo would not get reset.

@dannycoates , I am highly grateful to you for explaining me this. Thank you very much.

Yes

Thanks for this too.
No more doubts

@snyk-bot snyk-bot mentioned this pull request Feb 1, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dannycoates dannycoates dannycoates approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Restrict the banner from showing on unsupported browsers
3 participants
@himanish-star @dannycoates @shikhar-scs