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 improvements #2401

Merged
merged 25 commits into from
Nov 7, 2023
Merged

Vue improvements #2401

merged 25 commits into from
Nov 7, 2023

Conversation

powerpaul17
Copy link
Contributor

@powerpaul17 powerpaul17 commented Oct 28, 2023

Summary

I made some improvements to the new vue frontend:

  • use Nextcloud components for item list & article view
  • behaves better on small/narrow screens

Bildschirmfoto am 2023-10-28 um 23 23 20
Bildschirmfoto am 2023-10-28 um 23 23 40
Bildschirmfoto am 2023-10-28 um 23 23 46

Checklist

Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
@SMillerDev
Copy link
Contributor

Hi @powerpaul17, could you post some screenshots of the changes?

@codecov
Copy link

codecov bot commented Oct 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2245250) 89.28% compared to head (2d9f100) 0.00%.
Report is 7 commits behind head on master.

❗ Current head 2d9f100 differs from pull request most recent head 5410721. Consider uploading reports for the commit 5410721 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #2401       +/-   ##
============================================
- Coverage     89.28%       0   -89.29%     
============================================
  Files            67       0       -67     
  Lines          3239       0     -3239     
============================================
- Hits           2892       0     -2892     
+ Misses          347       0      -347     

see 67 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Copy link
Contributor

@devlinjunker devlinjunker left a comment

Choose a reason for hiding this comment

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

awesome work! i'm glad to have made the push on the migration which seems to have inspired you as well!

src/components/ContentTemplate.vue Show resolved Hide resolved
src/components/ContentTemplate.vue Outdated Show resolved Hide resolved

<div class="main-container">
<div class="title-container" :class="{ 'unread': item.unread }">
<span style="white-space: nowrap" :dir="item.rtl && 'rtl'">
Copy link
Contributor

Choose a reason for hiding this comment

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

should this style be moved to a css rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just moved, I did not change/add it.. but I can move it into a class if you like.. 😉

@powerpaul17
Copy link
Contributor Author

awesome work! i'm glad to have made the push on the migration which seems to have inspired you as well!

Yes, it's way easier once all the tooling and such is set up.. 😊 Thanks a lot for taking up (and finishing) the rewrite..

@powerpaul17
Copy link
Contributor Author

Also I noticed I have to fix the frontend tests.. 😅

@Grotax
Copy link
Member

Grotax commented Oct 29, 2023

Tested, works for me. I think good improvements.

The explore page seems to be visually broken now. No more background.

grafik

@powerpaul17
Copy link
Contributor Author

Tested, works for me. I think good improvements.

The explore page seems to be visually broken now. No more background.

grafik

I also noticed that, but wasn't sure if it is because of my changes.. but I'll have a look at it..

Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
@powerpaul17 powerpaul17 marked this pull request as ready for review October 29, 2023 19:38
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
@devlinjunker
Copy link
Contributor

is it possible when no item is selected for the details section to be hidden so we can just show the list of news items?

Rather than this empty space:
Screenshot 2023-10-29 at 7 37 35 PM

@devlinjunker
Copy link
Contributor

we'll also probably need to make a way to mark items as read without having to open them in the mobile view, or to navigate through the items, since they take up the whole screen now and you can't select another item without pressing the back button..

@powerpaul17
Copy link
Contributor Author

were you able to fix the explore page so it has the white background? can you post a screenshot of it if so?

Yes, I forgot to add the content wrapper to the explore page since I moved it out of the app skeleton into the routes. Already fixed it.

@powerpaul17
Copy link
Contributor Author

we'll also probably need to make a way to mark items as read without having to open them in the mobile view, or to navigate through the items, since they take up the whole screen now and you can't select another item without pressing the back button..

That occurred to me too, but I did not want to include everything in one PR but start slowly.. 😉

@powerpaul17
Copy link
Contributor Author

is it possible when no item is selected for the details section to be hidden so we can just show the list of news items?

Rather than this empty space:
Screenshot 2023-10-29 at 7 37 35 PM

I think there is a component for the empty view when no item is selected. Removing the details content would seem rather strange I guess.

@devlinjunker
Copy link
Contributor

we'll also probably need to make a way to mark items as read without having to open them in the mobile view, or to navigate through the items, since they take up the whole screen now and you can't select another item without pressing the back button..

That occurred to me too, but I did not want to include everything in one PR but start slowly.. 😉

Should we merge this to a branch then rather than master, so we can fix this before it is released?

I think there is a component for the empty view when no item is selected. Removing the details content would seem rather strange I guess.

Can we just hide the details if there is nothing selected? just seems like a huge waste of screen real estate

@Grotax
Copy link
Member

Grotax commented Nov 1, 2023

I don't mind merging it into master when you both are done with the stuff you want to cover in this PR.

I don't intend to release a stable until the front-end is "done" until then we will release alphas and people have to deal with the imperfections. Or stay on the last stable release..

They could even roll-back with some manual effort.

@powerpaul17
Copy link
Contributor Author

As I already said, I would do it in another PR as this one deals with the list/content view. I just want to address the empty content issue and fix the failing tests.

@devlinjunker
Copy link
Contributor

Sounds good!

@powerpaul17
Copy link
Contributor Author

@devlinjunker I had to remove the two failing tests for showing feed title and unread count because it doesn't work anymore with shallowMount. The header is also positioned absolutely at the moment which should also be moved into the right component but I did not want to do it now. Would it be ok for another PR?

Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
…t header

Signed-off-by: Paul Tirk <paultirk@paultirk.com>
@devlinjunker
Copy link
Contributor

that seems fine to me

@Grotax
Copy link
Member

Grotax commented Nov 7, 2023

Can we merge this or anything left for this PR?

@Grotax
Copy link
Member

Grotax commented Nov 7, 2023

Might need a rebase on master and maybe check of npm lock file, since the recently merged changes

@powerpaul17
Copy link
Contributor Author

From my perspective it can be merged. I would rather do further improvements in a separate PR.

@SMillerDev SMillerDev merged commit 2facfda into nextcloud:master Nov 7, 2023
17 of 21 checks passed
Grotax added a commit that referenced this pull request Nov 8, 2023
Changed
- Add support for Nextcloud 28
- Use Nextcloud vue components for item list and article view (#2401)
- Fix aspect ratio of article images (#2401)

Fixed
- Adjust search urls to match changed Vue routes (#2408)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax mentioned this pull request Nov 8, 2023
@anoymouserver
Copy link
Contributor

Apparently the main-container in FeedItemRow.vue is now dependent on the content width.
screenshot

@powerpaul17
Copy link
Contributor Author

Apparently the main-container in FeedItemRow.vue is now dependent on the content width.
screenshot

I'll take a look at it. Will do a follow-up PR soon..

@powerpaul17 powerpaul17 deleted the vue-improvements branch November 8, 2023 21:08
Grotax added a commit that referenced this pull request Nov 12, 2023
Changed
- Add support for Nextcloud 28
- Use Nextcloud vue components for item list and article view (#2401)
- Fix aspect ratio of article images (#2401)

Fixed
- Adjust search urls to match changed Vue routes (#2408)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Grotax added a commit that referenced this pull request Nov 12, 2023
Changed
- Add support for Nextcloud 28
- Use Nextcloud vue components for item list and article view (#2401)
- Fix aspect ratio of article images (#2401)

Fixed
- Adjust search urls to match changed Vue routes (#2408)

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