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

[Vue Rewrite] Basic Mobile accessible interface #2371

Merged

Conversation

devlinjunker
Copy link
Contributor

Related to #748

🗒️ Summary

  • Add Media Queries and styles that make the article appear at the bottom of smaller screens (rather than on the right)
  • Clean up a couple of other styles to make the article area appear more separate (box shadow + border)

✅ Checklist

- [ ] Changelog entry added for all important changes.

📷 Visual

On my Iphone:
10987

Testing on narrow width browser:
demo

Still working in normal width:
demo2

➡️ Up Next

  • Fix Explore Page (WIP)
  • Update Documentation for Front End
    • Developer
      • Frontend Tips/Organization
      • Front End Unit Tests
    • Sharing
    • Remove Frontend/JS Plugin Documentation ( should we just update this when we add the new addArticleAction Plugin API?)
  • Settings in Sidebar
    • Component
    • Backend Requests + Application Behavior Changes
      • Keyboard Navigation
      • Mark on Scroll
      • Reverse Order (Oldest to Newest)
      • Upload/Download Subscriptions/Articles
  • use ?selected=<id> url query parameter to store and parse which Feed Item is displayed in FeedItemDisplay (this will be useful for implementing the search bar - and allow navigating directly to the specific feed item)
    • Also need to update search provider php code
    • Update Documentation?
  • addArticleAction Plugin API
    • Nextcloud News Code
    • Add to Bookmarks Example Plugin
    • Add New Frontend/JS Plugin Documentation

Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
@devlinjunker devlinjunker added the Skip-Changelog No changelog update is required, minor change label Oct 5, 2023
@Grotax
Copy link
Member

Grotax commented Oct 6, 2023

Works for me, but I notice that the audio player seems to be gone (maybe already since the last PR)
And I have blank space at the top of the app.

screenshot

@devlinjunker
Copy link
Contributor Author

devlinjunker commented Oct 10, 2023

@Grotax which browser are you using? It seems to appear for me on firefox and chrome, and has no space at the top:
Screenshot 2023-10-10 at 12 47 12 PM

I didn't fully test the audio/video playback in mobile though... so i'll do that now

Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
@devlinjunker devlinjunker force-pushed the mobile-friendly-styles branch from a1df985 to f576eb4 Compare October 10, 2023 20:44
Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
@devlinjunker
Copy link
Contributor Author

devlinjunker commented Oct 10, 2023

I've made some changes to the css, using flex more properly rather than relying on media queries to set the height.

Let me know if it shows up now, and if it doesn't it would be great to include which browser it is not appearing in. I have tested on OSX chrome, safari and firefox and iOS chrome and safari so far

@Grotax
Copy link
Member

Grotax commented Oct 11, 2023

Maybe the issues are related, I tried again based on this PR.

In both Firefox and Chromium I have the empty space at the top and the player is not visible.

I build the news app with make
For nextcloud I use https://github.com/juliushaertl/nextcloud-docker-dev
I use the stable27 branch of nextcloud server.
Latest Chromium and Firefox on Manjaro Linux.

I will try on my VM that is built like an actual standard nextcloud server

@Grotax
Copy link
Member

Grotax commented Oct 11, 2023

Same on my vm.

Also in that context noticed that we will need to adjust the build process for the appstore a bit but that can be done later.

Not sure what else I can do, I also don't have much experience with frontend things.

@devlinjunker
Copy link
Contributor Author

devlinjunker commented Oct 11, 2023 via email

@Grotax
Copy link
Member

Grotax commented Oct 11, 2023

Same from a Windows 10 with Edge browser.

stable28 is not out yet :)

So i think it has something to do with building the app or the combination with the nextcloud version, some css rule not working with the servers css?

I will try with other nextcloud versions when I can

@devlinjunker
Copy link
Contributor Author

you're right, I'm using stable27 (:

I built the app on using make on my dev machine
Screenshot 2023-10-11 at 2 17 43 PM

and it still doesn't seem to have the css issue, I also tried dark mode and I'm not seeing it yet
Screenshot 2023-10-11 at 2 19 42 PM

I am using osx though for hosting and building ... so idk if that would cause any differences..

@devlinjunker
Copy link
Contributor Author

devlinjunker commented Oct 11, 2023

If someone else could test this that would be great to get a steps to reproduce

But until then it would also be awesome if @Grotax you could right click to inspect in the browser:
Screenshot 2023-10-11 at 2 22 24 PM

and then use the search feature of the inspector tab to search for .podcast and I think go to the 2nd instance of it. If you hover your mouse in the HTML DOM over the <div class="podcast" ..> element you should see where that is appearing in the page:
Screenshot 2023-10-11 at 2 22 36 PM

You can also use the inspect technique to right click in the blank space at the top of the screen, and it should navigate in the Inspector to show which element is causing that and you would have to try to parse the css rules on the right hand side of the inspector panel.

@SMillerDev SMillerDev mentioned this pull request Oct 12, 2023
8 tasks
@Grotax
Copy link
Member

Grotax commented Oct 13, 2023

After adding
#content {
margin-top: 0px
}

I can see the player again, so it was there all the time just hidden.
Also works in the mobile views simulated by Firefox, I think the break points are maybe not perfect.
And I noticed in the player that the buttons can get hidden, I think we will need a different design there.

But all in all works and based on modern tech so I like it.

We can merge this in my opinion but there seems to be some conflicts it requires a manual re-base.

@devlinjunker
Copy link
Contributor Author

@Grotax I don't see any conflicts

Screenshot 2023-10-14 at 10 39 28 AM

should I just merge?

@SMillerDev
Copy link
Contributor

Yeah, go for it

@devlinjunker devlinjunker merged commit 0a2ecc2 into nextcloud:vue-rewrite Oct 16, 2023
16 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip-Changelog No changelog update is required, minor change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants