This repository has been archived by the owner on Apr 1, 2020. It is now read-only.
Add the desktop app banner #1067
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
remybach
commented
Aug 18, 2017
remybach
commented
|
looks good once the css file size is addressed in the component as discussed. |
🐿 v2.5.16
remybach
force-pushed
the
rb-desktop-app-banner
branch
from
bb22f79
to
25a5a63
Compare
leggsimon
approved these changes
Aug 18, 2017
There was a problem hiding this comment.
Looks ok to me, a couple of points though, as discussed I don't like the negated flag name format. I would ideally reject this based on that but if you're already doing it then I'm happy to let it slide if we can discuss for the future.
Also, I'm quite surprised that the SASS from n-desktop-app-banner adds ~4kb 🤔
|
For posterity, @leggsimon's point about the added 4kb is due to |
wheresrhys
added a commit
that referenced
this pull request
Sep 4, 2017
… rhys/n-topic-search * 'master' of https://github.com/Financial-Times/n-ui: (42 commits) Add EventSource polyfill Don't initialise desktop app banner more than once Remove code for dead offlineToastMessage feature Slice nodeList and use every Add webhook Update README.md Prevent page view events for headline testing if multiple Tweak flag used to display banner (#1077) Updated o-ads Update floodlight to fix trial confirmation (#1075) Update floodlight.js (#1071) Remove ternary and attach headline variant to page only add es2016 and 2017 babel presets (#1073) simplify typeahead search link Fix typo in readme Name change for headline testing Add new advanced search flag Revert disableDesktopAppBanner flag check Check the `disableDesktopAppBanner` properly Add the desktop app banner (#1067) ... 🐿 v2.5.16
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.