-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support better reading in a tablet screen #467
Conversation
I appreciate the effort but:
I will wait for @SMillerDev but for me its 👎 |
I appreciate the effort but I don't actually see anything news specific here. I think all these fixes should be submitted to the nextcloud server repo. It'll make it better for a lot more apps then just news. |
Well, the issue is that these CSS rules are ok for other apps, like Files but not for News. The reason is that we have a lot of relevant text that should fit in one single line. In Files normally the file names are not that long and even if they get truncated it is not a big deal. @Grotax thanks for your comments. I am not familiar with NC development but I can address those issues if we want to go ahead with this. |
@nextcloud/designers any opinion on this? |
Yep, such changes should probably be integrated into server. Though I doubt we can aim to fit all the differents tablet settings. By deciding only two modes, we simplify our layout a bit. But if we add 1024px, we'll better remove the 768px limit. Having two sizes that do the same thing doesn't really make sense to me :) |
Thanks for not dropping the issue. The fact is that this doesn't look good on a tablet so something should be done somewhere. IMHO this is a News app issue, because other Apps look just fine (for instance, Bookmarks does line wrapping). That being said, if it makes sense for NC designers to switch the media query cut from 768px to 1024px that would also work and would be perfect. Also it would be a tiny change codewise :) Should I open an issue/PR with this in nextcloud/server? Please, point me in the right direction |
@jancborchardt there is a 2% diff between 768px and 1024, maybe we should indeed switch to 1024 🤔 https://www.w3schools.com/browsers/browsers_display.asp In the meantime this could indeed be addressed in news. In files for example we simplify the current view on sizes < = 938px.
|
@skjnldsv that seems good indeed! Makes the view much simpler. |
I created a PR here nextcloud/server#15199 for the server side Should I try to cleanup this PR and get it merged? I think it still needs fixing before that gets accepted |
There's nothing in this review that wouldn't affect other apps right? |
This should be solved in the server repository rather than single apps. We have or had similar discussions e.g. in the Maps app, Calendar app, and others. |
I planted a proposal in the server code -> nextcloud/server#15199 Still, news needs a little tweak. I pushed a change to this PR that depends on the server change. What do you people think about changing
? |
If that is all that's needed I'm fine with it. But is there no css var for this? |
The server var is |
I meant variables like the colors we use. |
Doesn't seem like news uses scss at all
|
Please, review this last change. This allows to use server global variables, which is a step forward IMO. I agree that it would be nice to use other server variables as well. https://github.com/nextcloud/server/blob/master/core/css/variables.scss Once the change is merged in the server, the fix will work automatically in News. Just rename and use variables -> https://docs.nextcloud.com/server/15/developer_manual/design/css.html |
Needs signature and a rebase :) |
Signed-off-by: nachoparker <nacho@ownyourbits.com>
This reverts commit 0bfaa6a. Signed-off-by: nachoparker <nacho@ownyourbits.com>
Signed-off-by: nachoparker <nacho@ownyourbits.com>
merged and signed. Still DCO complaints... |
e2bda14
to
7533895
Compare
dd0625d
to
b15a3a6
Compare
ok, I had to sign all the commits. Done :) |
This PR is aimed at better reading on a >1024px width tablet.
Because the screen is quite narrow, we can see how the titles get truncated.
After this patch, the sidebar collapses just like it does on the phone.
I replicated the relevant CSS code from
server.css
and changed the media query for the new screen size.