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

Bug 936401 - [Flatfish] The detail of notification information doesn't s... #13609

Closed
wants to merge 1 commit into from

Conversation

gasolin
Copy link
Contributor

@gasolin gasolin commented Nov 12, 2013

...cale correctly

@rnowm
Copy link
Contributor

rnowm commented Nov 13, 2013

@gasolin unfortunately we cannot do that. That would break full screen modals in the phone.
I suggest you to create a new css file in: shared/style/confirm/responsive.css, and add there the media query:
@media (min-width: 768px) {
top: auto;
left: auto;
right: auto;
bottom: auto;
}
You could also add there any other style you need for FlatFish.

@gasolin
Copy link
Contributor Author

gasolin commented Nov 14, 2013

Thanks for noting this.

Should we add a new responsive.css and explicit include the separate file,
or just append @media query in the same file which auto applied to all files?
any negative impact of this approach?

@rnowm
Copy link
Contributor

rnowm commented Nov 14, 2013

Yes, you have to create this new file (inside confirm folder). There's no impact in performance for linking multiple files.

@gasolin
Copy link
Contributor Author

gasolin commented Nov 15, 2013

I'd prefer append @media query in the same css file if there's no significant impact. So these styles will be auto applied to all files without extra work.
Or we have to check over all of the places to apply new styles for tablet, which tends to bring more works and potential bugs.

PR update with in-file media-query
(set top/right/bottom/left to auto does not work)

@gasolin
Copy link
Contributor Author

gasolin commented Nov 20, 2013

hi, @rnowm , how do you think about this approach?

@rnowm
Copy link
Contributor

rnowm commented Nov 20, 2013

@gasolin Sorry for being late answering you, I discussed this with @vingtetun, The idea is to split css files when possible.
I get your point, do you think it makes more sense importing the new responsive.css inside confirm.css?
Thanks!

@gasolin
Copy link
Contributor Author

gasolin commented Nov 21, 2013

@rnowm if we importing the new responsive.css inside confirm.css, it might affect performance because the css import only happens after the css file is loaded.
After quick search confirm.css, there are 47 files include this file, both via link tag and lazy_load. lazy_load script might not respect the css import as well.

I've read from book that If we import .css via link tag in html like
<link rel="stylesheet" href="confirm/responsive.css" media="(min-width: 768px)"/>
It still always be imported, no matter what media-query conditions we specified
(because normal browser allow user change the window size, so the css should be prepared).
So we got no benefit from performance to include separate files but need to load extra one file.

This css is just a beginning, we may need tweak many existing styles in shared/ (from current radar we'd plan to enhance headers, value selectors...) to make them works perfectly on tablet.
I think a separate responsive.css is not a scalable way

@vingtetun
Copy link
Contributor

My feeling is that at the end we may end up with different index.html pages to fit different form factors. As strong as I would like to be able to use media query only it seems to introduce a lot of work to have the exact same files when you really want a different layout.
But I based that on the assumption that we may want a different layout for tablet in the future, which is hard to predict since most of the UI mockups I have seen right now are more: recycle our mobile UI to use 2 columns on tablet. So that's why I tend to think that different css files would have been easier and less expensive at the end. But all this is based on how I imagine our future UI and not on current facts.

Also, even if the CSS file is still downloaded, I'm curious to see if it is parsed when you do the tag way (I would assume it is). Maybe this worth checking.

So whatever you decide here. I think it does not matter that much. I feel like separate files are better but I don't feel like this should slow us down in any cases.

@gasolin
Copy link
Contributor Author

gasolin commented Nov 27, 2013

@vingtetun Thanks for clarify your consideration. I fully understand your concerns. We've some discussion about the approach that to have separate index.html pages to fit different form factors or recycle our mobile UI to use 2 columns on tablet, and UX choose to trace the responsive design (recycle our mobile UI).

Another news is we just be informed that we'll focus back on phone and not distract to other devices after 12/9 FC. So we can expect there will be no (or not much) patch for other devices in 1.4. (If the plan is not changed)
In this context, I think minimum viable change is a practical way to quick clean the tablet only style issues.

@mozilla-autolander-deprecated
Copy link
Contributor

This pull request has been closed due to tree stability issues. Please rebase and re-open the pull request if you still need to land this. Ensure the gaia-try run is green before landing. Sorry for any inconvenience.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants