Skip to content

Fix Bug 935719 - Tabzilla: Create an out of date version warning for users not on the current version#1548

Closed
kyoshino wants to merge 1 commit intomozilla:masterfrom
kyoshino:bug-935719-updatebar
Closed

Fix Bug 935719 - Tabzilla: Create an out of date version warning for users not on the current version#1548
kyoshino wants to merge 1 commit intomozilla:masterfrom
kyoshino:bug-935719-updatebar

Conversation

@kyoshino
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: should always specify a radix parameter when using parseInt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Added the radix param for backward compatibility. (It's no longer necessary on Firefox 21+ and other modern browsers)

@kyoshino
Copy link
Copy Markdown
Contributor Author

kyoshino commented Jan 7, 2014

Changed the cancel message from "Later" to "Update later." as per a :Habber's comment.

@alexgibson
Copy link
Copy Markdown
Contributor

Thanks @kyoshino - I'm guessing we need to start with l10n for these strings now the messaging has been looked over on the UX side. @flodolo @pascalchevrel?

@flodolo
Copy link
Copy Markdown
Collaborator

flodolo commented Jan 9, 2014

I see three strings

"Looks like you’re using an older version of Firefox."
"Update to stay fast and safe."
"Update later."

The first two are identical to new.lang, last one is brand new. Am I missing anything else?

@alexgibson
Copy link
Copy Markdown
Contributor

Should this update bar be restricted to desktop only?

https://support.mozilla.org/en-US/kb/update-firefox-latest-version also only relates to desktop information.

@kyoshino
Copy link
Copy Markdown
Contributor Author

kyoshino commented Jan 9, 2014

@flodolo Yes, we have only one new string: "Update later."

Should this update bar be restricted to desktop only?

@alexgibson Hmm, is it better to show the bar also for mobile users and add a mobile-specific info to SUMO? An ESR-specific info also needs to be added to the article.

@kyoshino
Copy link
Copy Markdown
Contributor Author

kyoshino commented Jan 9, 2014

Will update the PR to exclude Firefox for Android for now as discussed in the meeting.

@kyoshino
Copy link
Copy Markdown
Contributor Author

kyoshino commented Jan 9, 2014

And about the string: we are discussing the messaging for ESR users in Bug 952549.

@kyoshino
Copy link
Copy Markdown
Contributor Author

Added a check for mobile. (Android and Firefox OS)

@alexgibson
Copy link
Copy Markdown
Contributor

Added a check for mobile. (Android and Firefox OS)

Thanks @kyoshino! Can we please also add a test for mobile using some Firefox for Android & Firefox OS UA strings? Also I'd be interested to know if strings like Android, Tablet and Fennec are present in the UA without Mobile?

Update:

Ah, I see from here that Tablet and Mobile are interchangable. Fennec is used in the Nokia N900 https://developer.mozilla.org/en-US/docs/Gecko_user_agent_string_reference

@kyoshino
Copy link
Copy Markdown
Contributor Author

@alexgibson Thanks! Added tests for mobile.

@alexgibson
Copy link
Copy Markdown
Contributor

On a related note, I have opened a separate bug here as we should probably update code elsewhere on bedrock to reflect Firefox detection on Tablets:

https://bugzilla.mozilla.org/show_bug.cgi?id=963570

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need Android in as a check here?

Looking at the current Firefox UA's we should already be covered just by checking for Mobile and Tablet, unless I'm mistaken.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Umm, Android is probably redundant. The older UA strings didn't have Mobile nor Tablet but checking for Fennec should be enough. I'll remove it.

@alexgibson
Copy link
Copy Markdown
Contributor

Ping @pascalchevrel - just checking in how are we doing on l10n for this change?

@flodolo
Copy link
Copy Markdown
Collaborator

flodolo commented Feb 19, 2014

I think this bug slipped out of radar.

We have the first 2 strings in new.lang, nothing similar to "Update later."

@kyoshino
Copy link
Copy Markdown
Contributor Author

We still need to discuss the messaging in the bug.

@kyoshino
Copy link
Copy Markdown
Contributor Author

Closing until the final decision is made in Bug 952549.

@kyoshino
Copy link
Copy Markdown
Contributor Author

Detached the Tabzilla.setupUpdatebar function from tabzilla.js as commented in the bug. Other sites now can utilize Infobar in the same way as the Update Bar when they'd like to show a custom notification.

@kyoshino
Copy link
Copy Markdown
Contributor Author

kyoshino commented Apr 3, 2014

I have to update the message and logic.

@kyoshino kyoshino closed this Apr 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants