Skip to content
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

Merged
merged 3 commits into from
May 12, 2019

Conversation

nachoparker
Copy link
Member

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.

before-ncnews

After this patch, the sidebar collapses just like it does on the phone.

after-ncnews

I replicated the relevant CSS code from server.css and changed the media query for the new screen size.

@Grotax
Copy link
Member

Grotax commented Mar 25, 2019

I appreciate the effort but:

  1. You ignored the indentation
  2. The media query's where sorted by size big -> smaller screen
  3. There are options in there that don't belong in the news app
  4. Even a cleaner approach would probably violate rule number 3 https://docs.nextcloud.com/server/stable/developer_manual/design/navigation.html#basic-rules

I will wait for @SMillerDev but for me its 👎

@SMillerDev
Copy link
Contributor

SMillerDev commented Mar 25, 2019

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.

@SMillerDev SMillerDev closed this Mar 25, 2019
@nachoparker
Copy link
Member Author

nachoparker commented Mar 25, 2019

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.

@Grotax
Copy link
Member

Grotax commented Mar 26, 2019

@nextcloud/designers any opinion on this?

@skjnldsv
Copy link
Member

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 :)

@nachoparker
Copy link
Member Author

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

@skjnldsv
Copy link
Member

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 :)

@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.

<= 938 > 938
dev skjnldsv com_apps_files__dir=_ dev skjnldsv com_apps_files__dir=_ (1)

@jancborchardt
Copy link
Member

@skjnldsv that seems good indeed! Makes the view much simpler.

@nachoparker
Copy link
Member Author

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

@nachoparker nachoparker reopened this Apr 23, 2019
@SMillerDev
Copy link
Contributor

There's nothing in this review that wouldn't affect other apps right?

@jancborchardt
Copy link
Member

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.

@Grotax Grotax closed this Apr 25, 2019
@nachoparker
Copy link
Member Author

nachoparker commented May 2, 2019

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 mobile.css to mobile.scss so this change can just be

--- a/css/mobile.css
+++ b/css/mobile.scss
@@ -1,4 +1,4 @@
-@media only screen and (max-width: 768px) {
+@media only screen and (max-width: $mobile-breakpoint) {

?

@nachoparker nachoparker reopened this May 2, 2019
@SMillerDev
Copy link
Contributor

If that is all that's needed I'm fine with it. But is there no css var for this?

@nachoparker
Copy link
Member Author

The server var is $mobile-breakpoint, but we would have to change news/css/mobile.css to scss if I am not mistaken so it can evaluate it through sass

@SMillerDev
Copy link
Contributor

I meant variables like the colors we use.

@nachoparker
Copy link
Member Author

Doesn't seem like news uses scss at all

css
├── admin.css
├── app.css
├── content.css
├── custom.css
├── explore.css
├── mobile.css
├── navigation.css
├── settings.css
└── shortcuts.css

@nachoparker
Copy link
Member Author

nachoparker commented May 3, 2019

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

@Grotax
Copy link
Member

Grotax commented May 11, 2019

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>
@nachoparker
Copy link
Member Author

merged and signed. Still DCO complaints...

@nachoparker nachoparker force-pushed the css-tweak-tablet-screen branch 2 times, most recently from dd0625d to b15a3a6 Compare May 11, 2019 15:02
@nachoparker
Copy link
Member Author

ok, I had to sign all the commits. Done :)

@Grotax Grotax merged commit 98bb125 into nextcloud:master May 12, 2019
skjnldsv pushed a commit to nextcloud/server that referenced this pull request May 29, 2019
skjnldsv pushed a commit to nachoparker/server that referenced this pull request May 29, 2019
skjnldsv pushed a commit to nachoparker/server that referenced this pull request May 29, 2019
skjnldsv pushed a commit to nextcloud/server that referenced this pull request May 30, 2019
nachoparker added a commit to nachoparker/server that referenced this pull request Jun 28, 2019
Grotax added a commit that referenced this pull request Aug 16, 2019
- Dropped support for Nextcloud 14 & 15 #494
- Switched to feedio 4.3 #494
- News now requires PHP 7.1 #494
- Removed UTF-8 warning (now included in server) #497
- UI imporvements #505 #504 #467

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Grotax added a commit that referenced this pull request Sep 19, 2019
- Dropped support for Nextcloud 14 & 15 #494
- Switched to feedio 4.3 #494
- News now requires PHP 7.1 #494
- Removed UTF-8 warning (now included in server) #497
- UI improvements #505 #504 #467
- Add the 'Accept' header to requests #525

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants