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

New layout fix, scrolling and keyboard shortcuts fixes #312

Merged
merged 6 commits into from
Aug 20, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Aug 3, 2018

Fix #311
Fix #271

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@anoymouserver
Copy link
Contributor

anoymouserver commented Aug 3, 2018

Thanks ... Your layout fix seems to work quite well, but for some reason some of the templates and mostly hidden parts (feed error, player) now will be displayed at every loading of the news app until everything is build up. Besides that the loading animation is off-center at the top left of the content part (I assume that it should be centered).

Edit:
Apparently horizontal scrolling also causes some problems such as that the content moves over the feed list on the left side. Therefor the right buttons (mark read, fav) aren't accessible.
Until now, the description in compact mode was always truncated to the width of the content box, so that no horizontal bar was displayed.

nextcloud_news

@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 3, 2018

Thanks a lot!

Where do you get the top black header on your screenshot? I never saw that! :/
Do you have any example of a feed where there is horizontal scrolling? :)

  • loading off center
  • errors showing on load
  • scrollbar on compact mode

@anoymouserver
Copy link
Contributor

anoymouserver commented Aug 3, 2018

The black header on the top right is the player which normally shows as an overlay if you play the audio of a podcast (css class .podcast, enclosed in <!-- ngIf: App.playingItem -->) but I see it nearly every time when I reload the page (Ctrl + F5) until the page is finished.
I also found out, that this player hides the content when activated. (example podcast feed (german): https://chaosradio.ccc.de/chaosradio-latest.rss)

Most of the feeds in my list, which have a longer summary have this scrolling issue (one for example is: https://www.kdab.com/category/blogs/qt/feed/).

If that's helpful: currently I'm using Firefox 61.0.1 (64-Bit) for Windows 10 on a Full HD monitor.
Same on Firefox 61.0 for Android.

Copy link
Contributor

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know a lot/anything about the JS part so I don't think I can really sign of on this.

@SMillerDev
Copy link
Contributor

@danopz you did some work on webpack, you probably have a better grasp of the JS part. Would you mind looking at this?

@danopz
Copy link
Contributor

danopz commented Aug 7, 2018

Yesterday i've looked a bit into it, will continue today.
I'm not that familiar with the JS part, but I believe I can understand most of it 😀

What I found so far is that the player + sidebar is just printed with PHP and gets displayed before AngularJS is fully initialized, so the HTML just gets rendered and will be hidden later.
In the current release this is the case as well, with the difference that the global loading animation is scaled to 100% of the screen, so the other content begins after 100% height (throttle your network speed and scroll down 😉)
So for this we just need to set 100% width+height, but I wasn't successful till now.

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@danopz
Copy link
Contributor

danopz commented Aug 7, 2018

Again I've looked through the branch and for me much of the frontend seems to be broken. Is it just me?

  • app-content is to wide
  • subscribe, new folder and settings buttons doesn't work
  • initial loading icon was full screen, but isn't anymore
  • there are scrolling issues (at least initial scroll height)

@skjnldsv
Copy link
Member Author

@danopz I'll dive in this today again :)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

@SMillerDev @danopz please test again :)

@tobiasKaminsky
Copy link
Member

Tested and working for me.
Only remaining issue for me is that auto mark as read is a bit weird on compact view.
@skjnldsv mentioned that he already is aware of it

@@ -35,7 +35,7 @@ Before you update to a new version, [check the changelog](https://github.com/nex
<lib>SimpleXML</lib>
<lib>iconv</lib>
<owncloud max-version="0" min-version="0"/>
<nextcloud min-version="13" max-version="14"/>
<nextcloud min-version="14" max-version="14"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this no longer work with the 13 version?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Behaviour is not retro compatible

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this will always make travis fail, maybe the branch bump for travis should also be part of this review (which means the final merger would have to wait for the stable14 branch to exist). @nextcloud/news anyone else with an opinion about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now you can just test against master until there is a stable14 branch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, or disable the old stable branches :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to have a stable13 branch for news? And from master then the 14 app release can be done?
So bugfixes for 13.x app can still be merged & released (if needed)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tobiasKaminsky I guess?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the past, Bernhard always dropped support for older server releases once a new major release was out. Doesn't mean it stil needs to be done like that, but it'd be easier and follows the known pattern.

@danopz
Copy link
Contributor

danopz commented Aug 14, 2018

@skjnldsv still not working for me. Now the left sidebar is hidden. Tested with Opera and Firefox. App is installed in Nexcloud 14 beta using the latest Docker image.
I updated the local branch and rebuilt it, don't know whats wrong, but don't have much time to investigate.

@skjnldsv
Copy link
Member Author

@danopz which nextcloud version are you on?

@danopz
Copy link
Contributor

danopz commented Aug 15, 2018

@skjnldsv 14.0.0beta3 (14.0.0.15)

@skjnldsv
Copy link
Member Author

@danopz Hum, it should work though.
have you tried clearing your cache?

@anoymouserver
Copy link
Contributor

I still have a horizontal scrollbar in compact mode.
Also there are two vertical scrollbars when opening the settings.

@danopz
Copy link
Contributor

danopz commented Aug 15, 2018

@skjnldsv I've rebuilt the docker container the problem still exists 😞
Browser cache should not disturb because of opened dev tools with disabled cache.

@skjnldsv
Copy link
Member Author

@danopz what about scss cache? Can you make sure you ran the maintenance repair?

@danopz
Copy link
Contributor

danopz commented Aug 15, 2018

@skjnldsv Didn't help

@tobiasKaminsky
Copy link
Member

@skjnldsv any news here? Can I help you somehow?
Would be cool to have a nc14 compatible release soon.

@skjnldsv
Copy link
Member Author

@tobiasKaminsky we can already merge this, the only issue left is the mark as read on scroll of compact mode.

@skjnldsv skjnldsv force-pushed the 14-bump-scroll-fixes branch 2 times, most recently from 6ef12e6 to edf708f Compare August 20, 2018 09:55
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the 14-bump-scroll-fixes branch from edf708f to 23abfed Compare August 20, 2018 09:57
@tobiasKaminsky
Copy link
Member

👍 works for me.

@skjnldsv skjnldsv merged commit 1df733a into master Aug 20, 2018
@skjnldsv skjnldsv deleted the 14-bump-scroll-fixes branch August 20, 2018 10:32
@SMillerDev
Copy link
Contributor

Thanks @skjnldsv !

@anoymouserver
Copy link
Contributor

Am I the only one still having a huge horizontal scrollbar in compact mode caused by the feeds description or long title?
Same for double vertical scrollbar while opening the settings.

# installed latest news master with
make clean
make
# plus following
occ maintenance:repair
settings_scroll

@skjnldsv
Copy link
Member Author

@anoymouserver on latest server master I don't.

@LEDfan
Copy link
Member

LEDfan commented Aug 28, 2018

@skjnldsv @anoymouserver I just updated to NC daily (commit 148e9cacb0d046d41896d3d5dab0b1a90bd1421b, which is only a tx update behind current master) and also have the huge horizontal scrollbar.

I also had this in the past (#245), this time removing display: flex from `#content# fixes it.

@hoellen
Copy link

hoellen commented Sep 7, 2018

Removing display: flex from #content fix the "huge horizontal scrollbar" issue for me. Thx @LEDfan

@ghost ghost mentioned this pull request Sep 11, 2018
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants