Add a mobile preview bar to Drupal core

This patch is not quite a Minimally Viable Product (MVP). But the basic interaction is present. I'll be iterating on it and posting progress patches.

Work is moved to #1880606: Introduce a configuration UI for theme-based breakpoints within the Breakpoint module queue.

...

This is still a work in progress. I think Wim just got a little zealous.

I wish we had a tag for "needs progress review" and "needs final review".

This patch is a major upgrade from #11. It brings the UI closer to the designs by tkoleary.

Those designs can be explored at https://projects.invisionapp.com/share/U4BPGASQ#/screens

I spent the day just getting the UI changes in place and cut some corners with code cleanliness. My next step is to go back and clean up and put in docs. I wanted to get to a point where folks can test the working UI even if it means the code underneath is a bit prototypy.

The latest changes can (always) be tested at simplytest.me with the 8.x-1.x branch.

Uncaught TypeError: Cannot call method 'toggleClass' of undefined layout.preview.js:64

It turns out the controls don't exist until you've previewed a device. I need to take that case into account. Thanks for the report.

This patch moves the preview code out of the Layout module and into a new module called Responsive Preview.

I started a project for this module here: http://drupal.org/project/responsive_preview

This patch greatly reduces the functionality of the preview. The developer ruler and size input have been removed. It is now only possible to preview a page at the pre-defined device dimensions. The developer features may be reintroduced at a later time.

Next steps

1. Small visual clean-ups.
2. RTL stylesheets
3. JavaScript review
4. Write tests

Large negative text-indents are not considered LTR anywhere else in Drupal core, so let's not introduce that here.

Replaced with .element-hidden spans.

The close icon is not yet being drawn on the left side of the preview in RTL situations. I didn't want to implement this because I'm afraid I'd forget about subtle aspects, thus potentially introducing more problems than I'd solve. This snippet should help you know whether the current page is LTR or RTL: jQuery('html').attr('dir').

Fixed by taking RTL/LTR into account when setting the edge distances for left or right.

+++ b/core/modules/responsive_preview/config/responsive-preview.devices.ymlundefined
@@ -0,0 +1,21 @@
+ desktop:
+ label: desktop
+ dimensions:
+ width: 1366
+ height: 768
I think this one can be removed? After all, you're going to be looking at this from "a desktop" anyway. Because previewing this device on an iPad is never going to work anyway.

The "standard desktop" is 1366px wide. On a large monitor, it's possible to preview this. If you're monitor isn't large enough, it won't appear as an option.

Do we even *want* this to appear on narrow screen widths?

36em seems to imply "smartphoneish" (because on an iPad, it still displays on the right).

Only on tablets and higher it makes sense to show this, IMO.

The only options you get are the ones your device is wide enough to preview. If the result is no devices, then the tab disappears.

You should put the rest of the attach() function's body in an anonymous callback passed in as the second parameter to .once().

If I'm wrong, then you need to document this better :)

once returns a jQuery set. You can either pass that set to a callback or store it in a variable. $body.length will be 1 on the first attach and zero on all subsequent attaches. It just saves writing another function and indenting everything another 2 spaces.

Why window.top and not just window?

There are a couple window objects in play here because of the iframe. The behaviors get attached to the page inside the iframe as well, so I'm just being really explicit about which window I want in all cases to avoid any silly mistakes.

+++ b/core/modules/responsive_preview/js/responsive-preview.jsundefined
@@ -0,0 +1,380 @@
+ // The list of options will most likely render outside the window. Correct
+ // this.
+ .drupalLayout('correctEdgeCollisions');
Can you clarify this? I don't understand *why* this will "most likely" render outside the window.

The CSS should prevent this from happening, so I changed the wording to "might". Whenever elements are absolute positioned like this, I worry that some part of the element will render off screen.

Why is this call necessary? loadDevicePreview() is already calling updateDimensions() with the exact same parameters.

Removing this line didn't make any difference and causes the number of calls to updateDimensions() to drop from 3 to 2 upon initial drawing.

Totally right. Good catch. I removed it.

Why is this .trigger() necessary? loadDevicePreview() is already calling updateDimensions() with the exact same parameters.

Removing this line didn't make any difference and causes the number of calls to updateDimensions() to drop further from 2 to 1 upon initial drawing.

Good catch again. The control flow didn't always go through loadDevicePreview. I removed a lot of code and these are some of the vestiges. Although something tells me they could have been redundant before too.

+++ b/core/modules/responsive_preview/js/responsive-preview.jsundefined
@@ -0,0 +1,380 @@
+ // @todo, are there any security implications to loading a page like this?
I don't think so, but I think as long as we do the same as overlay.module here, that we'd be good.

So: let's check whether overlay.module does the same.

I went though drupal.js and found Drupal.encodePath. So now the code is:

If you want to keep the structure the same, then drupalLayout should be renamed to drupalResponsivePreview.

Good point about the name, I missed that one during the refactor. I would like to keep this structure because these functions act on jQuery objects and it's just so easy to chain them. But it's polluting the jQuery fn namespace just for convenience. Blech, I need to rewrite them as functions. They still iterate over a set of jQuery objects...just not as a jQuery plugin on the $.fn object.

Blarg, it posted a file in #18 that I mark not to be listed.

This is the correct patch file: http://drupal.org/files/mobile-preview-1741498-18_0.patch

Please change hyphen to underscore (responsive_preview.devices.yml).

There's no responsive_preview_menu() in the patch, and this function isn't a page callback.

Needs a docblock.

Tested the patch. Some suggestions:

1. "Android" is an Operating System, not a device. Some Android devices to consider: "Kindle Fire", "Nook", "Surface", "Nexus 7".
2. When your browser window is smaller than the "Typical desktop" size, the preview is too small. That option only works reliably when you have a big enough screen and browser window.
3. I'd add "iPad Mini", to differentiate from a normal "iPad".

Suggestions for follow-up patches:

1. Visualize the height of these devices. Maybe we can render a line or something to denote the bottom of the screen.
2. Ability to flip between portrait and landscape mode.

1. "Android" is an Operating System, not a device. Some Android devices to consider: "Kindle Fire", "Nook", "Surface", "Nexus 7".
3. I'd add "iPad Mini", to differentiate from a normal "iPad".

I filled out the device listing.

2. When your browser window is smaller than the "Typical desktop" size, the preview is too small. That option only works reliably when you have
a big enough screen and browser window.

Only the options that will fit in your current browser window, the gutters taken into account, will be shown as options to preview. It's true, most folks won't see the typical desktop option. On a Cinema Display, it will probably be available.

1. Visualize the height of these devices. Maybe we can render a line or something to denote the bottom of the screen.
2. Ability to flip between portrait and landscape mode.

We'll just need to discussion the design of the controls. The data to do this is all there.

The collision detection I had in the patch before just wasn't working with long lists, so I replaced it with jQuery.ui.position, removed a bunch of custom code, and everything is working well now.

Tagging for mobile. Also raising to major, given #27. I agree that achieving the goal of D8 being mobile-friendly is not just about good responsive design, but also making it easy for content creators to preview their pages in common device widths.

There is little people can do with this module

Well, after previewing for a particular device and seeing that the page doesn't look good, the content creator could edit the text in the node to be less wordy, or replace images with smaller ones (or configure better image styles for the <picture> element once we have that working), or if the content creator is a single blogger, perhaps browse the web for a theme that better suits their content, or if the content creator is part of a team, ask the site builder for help on how to improve the way the page looks on different devices.

I agree that the original design with the ruler, integrated breakpoint configuration, and responsive layout builder, would have made this better, but sadly, it didn't make it. I still think what's here is useful though.

Only the options that will fit in your current browser window, the gutters taken into account, will be shown as options to preview.

That wasn't the case with #23, but is fixed in #26: thanks.

#26 looks great to me, but it's 90% front-end code, so I'm not the right person to RTBC it. Hopefully a front-end developer can come along and do that :)

Something that hardcode a list of stuff is definitely not worthy for core inclusion in my opinion:

Also, all those device names are trademarked. So I assume we need a place to recognize the trademark somewhere.

This would be a great contrib module, thanks for all your hard work. But in core? Really, no.

Re: #30: Trademark should not be an issue as, to the best of my knowledge, this falls under nominative fair use.

The current patch confuses physical pixels and device-independent pixels, which seems to show a deep misunderstanding of how modern mobile devices actually work.

Let's give this time to mature in contrib, as it should. We have enough half-broken things in Drupal 8 already.

re: #34

The current patch confuses physical pixels and device-independent pixels, which seems to show a deep misunderstanding of how modern mobile devices actually work.

Wim Leers bent his brain around this problem most of the day yesterday. I'd struggled with it to for some hours, but this clever lad figured out a way to incorporate pixel density into the sizing. Truly inspired code! He also culled the list of devices considerably from #26 so there isn't so much overlap in sizing and device.

The current trend of core JS is moving towards Backbone for managing model-like state and the views of those models, so I switched the code here from closured-class-like structure to one a Backbone structure. It ends up being much cleaner and easier to follow.

I added a black-and-white set of borders to distinguish the previewed site from the modal background to address Bojhan's critique of the lack of contrast for darker background colored sites in #29-2.

The mobile preview button now glows blue when the preview is active.

The preview is now constrained by the device height as well.

#36: mobile-preview-1741498-36.patch queued for re-testing.

#39:

Browsing through the code, I do not think that getting into the pixel density business is a good idea. We are most likely not going to be able to emulate media-queries for device-pixel-ratio so that they work properly in the simulator.

Emulating those media queries is indeed impossible. But that's not why this information is in there.

It's there because it makes it easier to deal with devices that have "odd" dppx values, such as the Nexus 7. That device's width is 800 pixels, but it reports 603 pixels, in other words, it has a dppx ratio of about 1.3 (that's what pieroxy indicated and the value that I used, but obviously it's wrong, because 800/603=1.3266998342 — turns out Wikipedia pegs it at 1.325, which yields 800/1.325=603.77535. I've updated it accordingly).

So, in order to not have to list the Nexus 7 as a 603x966 pixels device, which looks extremely weird, I figured it'd be better to have slightly more metadata, to keep it sane & clean.

I think we should avoid naming devices all together.

When I look at this in a logical, puristic manner, I completely agree with you. However… reality is that most people would expect to see familiar names. "Small phone" doesn't ring any bells.

If you want to simplify it further — which I think is a good idea again from a logical POV — then I'd suggest to remove the Android devices and only retain the Apple devices. Pretty much everybody knows what the dimensions of an iPhone and iPad are. Their brand names have been around for years. There's so many different Android brand names, device names, resolutions, pixel densities and so on that even including the flagship (i.e. best known) devices is going to cause users of this functionality to wonder what these devices are.
(I've never seen a Nexus 4 or 7, for example. They're far from common in Belgium.)

For now, I've kept this the same, because I expect some bikeshedding here :P So, I propose that we defer from discussing the specific device list to use to a follow-up issue. That's a matter of choice, it's not a technical problem.

For all the non-desktop sizes, we also need to be able to preview portrait and lanscape orientation.

Agreed, but I was hoping that could be a follow-up as well.

#36:

• Fixed a bunch of comments.
• "current URI generation" was incorrect.
• Changed dppx for Nexus 7 from 1.3 to 1.325
• Unprefixed (removed leading underscore) the Backbone Views event callback functions, because that's against the Backbone coding style
• I split the model you were using into three models: the TabStateModel remains largely unchanged, but I moved the unrelated attributes out of there, into PreviewStateModel</a> and <code>EnvironmentModel (for the text direction and viewport width — I got rid of the parent window reference). This allows for much more clarity and better separation.
• Similarly, I renamed inconsistently named variables and methods. E.g. "devices" and "options" were used interchangeably, now it's consistently "devices", which makes it much easier to guess what a method is going to do.
• I also refactored some code to leverage Backbone more cleanly: instead of having document/window-event bindings inside Backbone Views, I moved those out of Backbone Views and into the setup code, which then update the model (i.e. set EnvironmentModel's viewportWidth). This allows for better decoupling and as dumb-as-possible Backbone Views: they should simply respond to model changes as much as possible, and do as little else as possible.
• Refactored the scaling code that I wrote for better understandability, and significantly extended the docs (including more useful references for people who also want to understand how it works).

Reviewed the PHP / module code. Only found one minor typo in the .info. Also a doc suggestion for extensibility.

"the a page"

If the suggested process to change devices or add more devices is that this single yml is changed, then it would be great to document. That is how the code currently "allows" device changes. I guess contrib can have a module to allow device list editing based on this yml key.

I don't know why bot is so slow on #43, but clearly, nothing in the interdiff could have caused that or broken anything, and #40 is both green, and the review log shows that it took the normal amount of time.

Jesse and I updated the issue summary and filed all follow up issues that have been raised here (I think, though please add more if that's not the case).

Although I'm not a front-end developer, I looked over the code, and while I don't deeply understand every bit of it, it does look well structured and scoped, so I'm confident that whatever problems surface can be resolved.

I also manually tested this again, and did not find any problems other than what we already have follow ups for.

What does / doesn't belong in core is often contentious, but ultimately is for Dries to decide. Assuming Dries commits this, if someone still feels it should be removed from core, please open a follow up to make that case. Dries often listens to solid arguments, and has several times removed things from core that he originally wanted.

I just feel like we are offering a preview option with actually very few useful tools to positively influence that preview.

We'll need to determine what other tools are in scope in #1920564: Decide what else is in scope for responsive preview and ensure contrib can extend what isn't, but even if the answer is nothing, I still think a preview is useful. As I said in #28, for sites where the content creator is also the site builder, seeing the preview could lead the person to any number of decisions from configuring image styles and text formatters to selecting a different theme. For sites where the content creator isn't the site builder, even if all the preview accomplishes is letting the content creator discover that there's a problem, and then all the content creator can do is inform the site builder to fix it, that's still a useful outcome.

Given all this, RTBC.

I am definitely concerned this is a bit of a skewed review process, there have been two non-spark reviews that both identified serious issues that are largely bumped to followups. Although I appreciate the work that has gone into this, it has been very late and left little room for involvement of the larger community.

I am not sure if committing this now is a good idea, we have received little to no feedback of the people involved in mobile besides jesse, this had no a11y review, it adds a optional module that has no clear use cases as pointed out (@effulgentsia you didn't seem to cover use cases that content creators can influence?), and a lack of useful site builder functionality. It would be useful if we have a plan for this, but this proposal is just not mature - like other proposals of Spark have been.

Anyways,- you can commit it, but its exactly situations like this that people have been afraid of - because even though valid points have been raised they are not really discussed before commit. I'm fine if this is the process for Favorites-of-Dries, but its not one that encourages larger community involvement.

@webchick I agree that's its a good idea, but I am just not sure about the current state - I know we are all before feature freeze and all but we did not have enough reviews to evaluate whether we are anywhere near MVP. Anyways just warning for Dashboard like situations, where we rush features in because Dries likes it - and then no one is interested to improve it, although Spark has committed to working in the cleanup phase - their list of things to solve has become incredibly long with quite some majors and lose-ends in both the edit-in-line, toolbar and now this module.

I imagine good improvements/followups would be:

1) Although content creators are advertised as the primary target audience, for them this will primarily be a "escalation" tool, to see that their article sucks in a certain device resolution. Perhaps for this purpose, we should add some "text" that is useful for the content creator to relay to the site builder (e.g. Iphone, Landscape mode).

2) We advertise that Drupal is thinking "mobile-first", but we currently in the UI offer little to no useful sitebulder tools around mobile. I think that means that we should try to bubble up some of those tools into this UI, like initially proposed with the theme break points, that is useful information for site builders. However clearly that cannot be shown to content creators, so we will need a way to disclose that more nicely.

3) I know the follow up to this will be to enable it by default. I would be against that until we figure out our toolbar strategy, we can't just keep pushing stuff to the top level. We will now have 3 icons on the right, all with different behaviors (dropdown, activate buttons all over, and tour). We are now near clean-up, we should not treat the toolbar like something we can endlessly stuff things in.

I guess I just like features to be more mature, when they get into core - in terms of review process, that hasn't happened here. I hope you understand, that will always be a concern of mine. Because in the end, if features are not actually useful (like Dashboard) to our larger audience, it is a negative bump in our UX for a full version (affecting in D7's case, at least 500.000 people) - even though the idea itself is really good (like this idea is really good). Its my role to avoid that from happening, a thorough review process is a significant way to do that. Up until 14 February, I did not take this serious - after all, we had many immature features rallying towards feature freeze. I imagine many other contributors felt like that.

But I also think the content creator / site builder blended role (like a single blogger) is an important one to consider, and here again, being able to quickly preview while in content creator mode, and then upon seeing a problem, putting on the site builder hat and figuring out a solution is equally useful.

I think with this hat on you want more information, like the theme its break points. I understand why we initially removed that, after all core is no theme debugger. But it seems like the biggest site builder part? Can't we have like a tiny "information" area - where we display the selection in text, and its meta information (breakpoints)? Especially on the mobile display there would be the space for this, possibly we can hide it behind an icon or something a like.

I agree, largely core features are evaluated by "does it make it worse", I always evaluate by "is its current form desirable, if Drupal was released tomorrow". That comes from previous experience, of many things that didn't get improved after they got it - but perhaps my judgement is clouded by Drupal 7 period which was incredibly hard on contributors who had to fix all of the stuff we pushed in.

#50

Can't we have like a tiny "information" area - where we display the selection in text, and its meta information (breakpoints)? Especially on the mobile display there would be the space for this, possibly we can hide it behind an icon or something a like.

Bojhan, yes, enriching the UI with more information is definitely possibe. This code existed in previous patches and Kevin has preliminary designs to that effect. It was removed to simplify this issue and bring this closer to a feature MVP, to be improved in subsequent patches. As you note, we'd want to hide that rich info behind a toggle or a button so the initial experience is just one of a quick preview with the possibility to experiment and manipulate right beneath the surface.

Here's the prototype link:

https://projects.invisionapp.com/share/JVBPENHP#/screens/4867869

Hey, everyone!
Just want to be sure you know about the modules we have developed some time ago:
http://drupal.org/project/mobileadaptivetest
http://drupal.org/project/adaptive_layout_tester

It has similar idea and maybe it can be useful to you. We will be happy to help you anyway!

Wooow, this makes me a sad panda...
I didn't knew about this issue untill I read Dries his blogpost.

1) The patch is rushed in a couple of weeks
2) Like Sun said, there are tools for that in the browser that work a lot better.
3) It doesn't fix any bugs and it's a completly seperate module. It could be contrib.
4) It doesn't mean it's marked as spark that it should be in core.
5) It doesn't help content editors. It will give the impression those are the only screensizes. And seriously is someone going to check every state when adding content
6) It doesn't help the sitebuilder because yeah they use the more performant and flexible browser tools anyway.

Maybe some decent independent usability tests could tell us some more.

Please reconsider adding this to core... (at least give the impression that it's not yet decided)
This will frustrate some people (including me) if this get in without a good reason.
I still wonder why I ever would use this module?

What does / doesn't belong in core is often contentious, but ultimately is for Dries to decide.

Well in this case Dries his spark agenda always will say *add this to core*.
So I believe we must give others the opportunity to react to this issue and convince him that it's not a good idea after all.
(with good and valid arguments)

*spark profile material*

ps: I'm not sure how "hard" my post sound (for native english speakers). But it's not ment to be hard so please don't think I'm being an evil guy here.

@LewisNyman: when is the last time you used Drupal core as a demo in your sales meeting? Being in core or not is totally irrelevant to the sales question.

On the other hand, putting something half-baked in core is a really good way to increase our technical debt. We have enough of that, we should focus on cleaning-up the existing debt, not add more.

Response to review

#55-1

I wonder how many more different close icons we want to introduce in Drupal core?

I brought this up with Kevin and he's given me a new image.

#55-2

A configuration object whose name contains "devices" does not need a "devices" key.

Good to know. Updated.

#55-3

That's an awful lot of custom styling, for a toolbar widget type (a dropdown list) that should be supported natively by Toolbar.

A fair point. At the moment, there's only one example of this type of dropdown -- a dropdown that isn't quite a dropbutton because it doesn't have a primary action. It's more like a select box, but select box would take even more styling to undo the native form element appearance, so I just went with a button followed by a list. If we see more instances of this type of element, I agree it should be made a type of display element that the toolbar offers. At the moment, it's custom code here or custom code there and it seems keeping the changes isolated to this module is just as well for the moment.

#55-4

These are not within the module namespace.

Right, holdovers from a module name refactor. Updated.

#55-5

Why don't you use an item list instead of links, if you don't want to output links?

Great suggestion. Updated.

#55-6

I don't understand why this condition exists.

One of our assumptions is that this feature is useful for content editors, not site builders. There is therefore no need to offer a preview of an admin page.

Changes

1. Updated the design to reflect the current prototype: https://projects.invisionapp.com/share/ZHCCF6W6#/screens/4867866
2. Added orientation rotation. #1920454: Add an orientation toggle to the responsive preview feature so that a page may be previewed in portrait and landscape modes can be closed.
3. Responded to comments from sun in #55.
4. Moved icon css to an icon.css file
5. Removed id selectors from the CSS files.

Could we check if the following is true?

The official Android documentation and various internet resources still say that the default viewport size on Android is 800px. I haven't found any mention that it changed in Android 4.0.

Removed by mistake "RTBC Feb 18th"

I don't know what the normal process is about adding something like this to core. Personal i don't really have a strong opinion. but it feels like comments / serious concerns of sun / aspilicious / bojhan are being neglected here.
Could be i'm missing discussions in IRC, but it doesn't seem resolved from a 'i just read this issuu'-perspective.

The last submitted patch, mobile-preview-1741498-67.patch, failed testing.

converted info file to info.yml

#70: mobile-preview-1741498-70.patch queued for re-testing.

The module EnableDisableTest seemed like a non-legit fail, hence I retested it in #72, but it seems it's legitimate after all. It's probably related to the .info to YML conversion issue?

I created an issue for testswarm, #1937240: Convert info file to yml

#70: mobile-preview-1741498-70.patch queued for re-testing.

yay, more useful information!

@aspilicious: that would also be a great argument for those who said above that browser plugins and other 3rd party desktop tools should be used for this, I guess. Those would not reproduce the real-real environment of the mobile device either. Let alone the touch controls vs. the mouse based navigation if touch detection features are used, etc. That would lead us to never attempting to support this since its not possible to be perfect, and just say to people to buy a piece of each device.

Yeah to me the big win on this isn't a sales gimmick (not sure why that assertion is being made?) but because it actually empowers people without $14,000 in hardware laying around to get a reasonable idea of what their site is going to look like on numerous mobile devices in about 30 seconds flat.

Is it a "perfect" preview? No, of course not. But I don't understand why someone would expect that either. The frame border is clearly generic, and all it's doing is window sizing (actually a little more than that). If we were shipping with device chrome like several other proprietary CMSes do, then possibly, but that's not even an option for a GPL project.

Seriously, just apply the patch and play with it. It's an extremely small and unobtrusive feature, but also really handy and practical. I'd love to have something like this on Drupal.org when developing some of the larger landing page types of things.

Yes gabor if you want to do proper testing that is the case. If you need to support a new windows tablet you probably have to buy one to be sure it works. If you do want a preview for developer reasons than I would use browser tools in stead of these javascript tricks as they are "slow".

Tech people know those are just "previews", management does not. So thats why I say: Don't use it to sell Drupal, which is the primary reason if I read all the comments above. Because they remember those tools and want to use them.

@webchick: Please be kind enough to read the points of the other parties, before trying to drown them under a wall of text.

Again, nobody is arguing against the usefulness of this feature. What's on the table is whether is belongs in core or not.

And because:

• It is a really new/immature development (as far as I understand, it started being developed in December; it still had *major* flaws right before feature "completion" phase),
• It is a standalone feature with zero adherence to anything else in core or contrib,
• It is has the potential of being deceptive (that can be resolved by avoiding listing actual device names but listing device types instead, as I already suggested),
• It has unresolved UX issues, especially around the target audience of the feature and its relationship with other forms of previews

... I'm arguing that this belongs in the contrib / distributions land for now.

This patch removes the testswarm dependency.

Thanks jibran for converting the info file to yml.

The last submitted patch, mobile-preview-1741498-90.patch, failed testing.

If devices can be defined using @Plugin annotation

It is something similar to how views handler are defined in D8. In D7 a hook_responsive_preview_devices() which can be used to define new devices.
I don't want to get of track here we can discuss it in the follow up if someone likes the idea.

#94: AFAIK the current viewpoint that Jesse and I hold is the one put forward by Dries, and it boils down to this: "no need to make this more complex than it needs to be". The list of devices is defined in a config file. If you want to modify the list of devices, then just modify the config file. That's what the config system exists for :) I don't think it makes sense to have a plugin type to define devices: 1) there's no logic involved, only metadata, hence it'd be massive overhead, 2) it doesn't really make sense for *modules* to provide new devices; each site owner decides for himself which devices he wants to support, and hence which ones he'd like to preview in. Therefor a configurable list of devices makes most sense.
I hope that adequately answers your questions :)

I just caught up on this thread after being away for the past few weeks. Just responding to the discussion of what the benefit of this being in core is. Particularly:

It is a standalone feature with zero adherence to anything else in core or contrib,

Which is often a very valid reason for something to be in contrib. However, one could also claim that being able to preview a node at all before saving it is a feature that from a code architecture standpoint, could live in contrib. And also suffers from being potentially deceptive, has UX problems, and many developers, including myself, never use. But the reason node previews are a core feature is because being able to preview content before saving it, for many people, is an essential part of the content management process.

Similarly, I would extend #87 with an assertion that given the complete paradigm shift of mobile devices being in wide use already and especially in the next couple years, that someone with just a Drupal core installation not being able to quickly and seamlessly preview content in a range of sizes before saving/publishing it will seem as ridiculous as if you couldn't preview at all. As ridiculous as not being able to add images to posts (which we finally fixed in D7) and not having a wysiwyg editor (which we finally fixed in D8).

I sympathize with the concerns of this not having had time to get refined in contrib, but I also think that applies to much of what the mobile initiative has had to deal with: it's a result of everything related to mobile web technology changing so fast.

Maybe for one reason or another this won't make it in, but for the reasons above, I'll be a bit embarrassed about D8 if that's what happens.

@Damien: Ah. Well, good to know you're being consistent in that regard :)

While working on backporting this module to D7, I added a block that launches the previewer so that the module wouldn't need to depend on the Navbar. I forward-ported the block to D8. Both the block and the toolbar components share a Backbone model, so their state is identical independent of which component is interacted with.

We'll need to move the testswarm tests to the FAT module if this is committed to core. For now, I'm leaving them in this patch so they are more convenient.

#102: mobile-preview-1741498-102.patch queued for re-testing.

@Jesse, what needs to happen to get this to a committable state?

This is a cute feature in its current state, I think I was wrong to fight it that much.

I have no idea what @chx means, but I am guessing its something about the state of Drupal.

Anyways, reviewing again. I think the patch is bugged, I do not get what jesse shows in #67. Could we get a reroll? The toolbar overlaps with the information part, there is no way to return to ordinary view, the arrow jumps around (do we really need an arrow?), there is a gray block on top of some previews (iPhone 4), there is useage of a checkmark in front of the selected option - this is quite new to Drupal, could we use a background color too?

I still do not get why you are so keen on adding specific devices, I understand it is easier to comprehend, but its just so fragile. Because you will need updates, to make sure they are up-to-date, as sizes changes, new mobile phones get more popular, it is currently very skewed towards Apple (there is no android tablet), there will be large potential for this list to become convoluted as we constantly add new ones (do we have rules on removing old ones?), and users are going to wonder how to preview it on every other device. I have not yet seen how we deal with this. It's all in .config to me, is just blowing away concerns and not actually addressing it, because we end up with a list that due to its code simplicity will be misleading to users in the long run.

I am happy we didn't rush this in, and allowed for further refinements because the meta data provides very useful information content creators can relay to site builders. @effulgentsia I think the thing that you lay it out clearly; the reason this got so much backslash is because it was rushed, and that always upsets people - especially when concerns are met with a lot of resistance. I think a lot of features have gone in, with loads and loads of UX followups (toolbar, edit in line) - sadly, few of them are going to get resolved before release, I think thats why me and probably also others push back on rushing things. Because in the end we need to solve those issues, and many of them are not within the resource reach of the Spark team.

Once we get a reroll, I will do a visual review - I have a few ideas, that might make it look nicer.

I've addressed some bit-rot that set in since #102 was posted and we got Drupal.displace committed. The preview window now positions its top edge using the latest Drupal Position-Rite Technology™.

I also added an Esc key press handler so the preview can be closed with the keyboard in the case the Bojhan encountered where the close X button is unreachable.

Kevin gave me a cleaned up icon sprite, so I've incorporated that as well. I haven't made any changes to the icons themselves.

I still agree on this, as mentioned several times already. I think a simple Phone / Phablet / Tablet would be (1) more manageable, (2) more future proof, and (3) less deceptive (because we don't pretend to do a preview of how it's going to look on an actual iPhone).

In many ways I'm just the developer on this issue and I can't make this call on my own, so I haven't touched this code. The good news is that making this change would just be editing a configuration file, so it won't be difficult to alter.

The last submitted patch, mobile-preview-1741498-111.patch, failed testing.

I gave this patch a try with the latest core head today, and it still works as good as ever.

@mgifford And the Spark team needs to coinside with the feedback that is opposing the use of device specific options, and I still get some bugs around the information?