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 #748

Merged
merged 135 commits into from
Oct 24, 2023
Merged

Vue rewrite #748

merged 135 commits into from
Oct 24, 2023

Conversation

JonathanTreffler
Copy link

@JonathanTreffler JonathanTreffler commented Aug 7, 2020

fixes #195

TODO:

  • add config files for webpack, eslint, stylelint, babel ...
  • planning how exactly to split the code (folder structure ...)
  • update Makefile
  • update CI Stuff
  • install all needed npm dependencies
  • Figuring out the data Structure (vuex)
  • Choosing the UI
  • Implementing the UI

@Grotax
Copy link
Member

Grotax commented Aug 7, 2020

Thank you so much for working on this 😊
Trust me it will be greatly appreciated not only by the active community on GitHub but also by lots of silent users.

@SMillerDev

This comment was marked as outdated.

@JonathanTreffler

This comment was marked as outdated.

@JonathanTreffler JonathanTreffler marked this pull request as draft August 7, 2020 10:13
@JonathanTreffler

This comment was marked as outdated.

Copy link
Author

@JonathanTreffler JonathanTreffler left a comment

Choose a reason for hiding this comment

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

Thanks VS Code for this Commit. No idea what happened there (with the commit title "�[")

@jancborchardt

This comment was marked as outdated.

@JonathanTreffler

This comment was marked as outdated.

@JonathanTreffler

This comment was marked as outdated.

@powerpaul17
Copy link
Contributor

Hello and thanks for starting this PR! Do you plan to simply reimplement the current interface or is there already a new design planned? Because I think there is a lot of room for improvements especially for a "reader" app. Personally I think the current layout/ui is not very convenient.

I would be available if you need help with the redesign/implementation..

@jancborchardt

This comment was marked as outdated.

@JonathanTreffler

This comment was marked as outdated.

@powerpaul17

This comment was marked as outdated.

@powerpaul17
Copy link
Contributor

@JonathanTreffler Sounds like a good plan, I was just thinking about a three-pane layout, like most reader apps have, the scrolling with the article interweaved is a bit confusing IMO. Also the typography is not very good at the moment. But of course, one step at a time. Let me know if I can help you with something.

@JonathanTreffler
Copy link
Author

I only use Nextcloud News so i have never seen a three pane layout, but it definitly sounds interesting. I think thats a layout mode we could add

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>
@SMillerDev
Copy link
Contributor

e16ab4a seems to create the issue that Grotax described in #2371 (comment).

@scottdotau do you maybe have some time to help debug that one?

@scottdotau
Copy link

e16ab4a seems to create the issue that Grotax described in #2371 (comment).

@scottdotau do you maybe have some time to help debug that one?

@SMillerDev @devlinjunker Sure, I'll take a look - apologies for going MIA, have had a few things come up over the weekend. Give me an hour or so and I'll look into this, and get cracking on some other stuff.

@Grotax
Copy link
Member

Grotax commented Oct 12, 2023

Found the css rule that I think gets imported from Nextcloud (maybe we have our own rule that already adds some margin top?) Might have also changed with some nextcloud version. I follow the stable27 branch and I'm currently on b75cb58aff6372b7a6ba51dfafabde9aec7b8e50

Peek.2023-10-12.08-24.mp4

@scottdotau
Copy link

scottdotau commented Oct 12, 2023

Just following along on commute home on my phone so can't copy line links for some reason but I believe what's happened is:

In the commit you mentioned @SMillerDev @ e16ab4a

src/main.js @ L26

  • Selector changed from id 'content'

templates/part.content.warnings.php @ L28-30

  • Still using content selector to remove margin (I assume?)

I have no proof that this is the case atm but will check when I'm home, however I'd assume the selector was changed from content because it's already being used for a parent container (I only know this coz of that screenrec)

Changing the selector in part.content.warnings.php should fix it, you can probably easily test this by adding #q-app { margin-top: 0 } in dev tools.

I'll also format this properly haha.

I'd assume the reason this pops up for you and not @devlinjunker, is he has a misconfigured cron and you don't... Also this is a weird place for the #content style dec to be considering the component itself doesn't doesn't use that selector imo

@Grotax
Copy link
Member

Grotax commented Oct 13, 2023

adding #q-app { margin-top: 0 } didn't change anything as far as I can see.
Adding #content { margin-top: 0 } removes the gap :)

If I search in dev tools the html for q-app in that commit it doesn't find anything

@devlinjunker
Copy link
Contributor

devlinjunker commented Oct 14, 2023

Thanks for helping debug @scottdotau and @Grotax! I think we should be using a different selector for initializing the Vue app. It seems like other Nextcloud applications set the el selector to #content so we should probably do that also

When I do that though, it seems like that causes the cron warning generated by PHP to be removed though... so I just need to look into how to re-use the Vue App error/warning code for displaying the cron message.

I've also been working on updating the documentation a little bit, so I'll try to combine those into the same PR in the next couple of days. Ideally we can release a beta before the end of October, I'll be going back to full time work then and will have less time to dedicate to this.

[Vue Rewrite] Basic Mobile accessible interface
Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
…an be committed

Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
@SMillerDev
Copy link
Contributor

@devlinjunker we're only missing the settings for feature parity right?

If so I'd suggest we make this an alpha release now so we can have more people's eyes on it. Then after the settings we can call it beta.

@Grotax
Copy link
Member

Grotax commented Oct 19, 2023

I also thought about that.

Alpha would be fine I think.

We just would need to merge this somehow 😁

@Grotax
Copy link
Member

Grotax commented Oct 19, 2023

Ah we still need to update the make file, the copy process of the JS files does not work anymore

@devlinjunker
Copy link
Contributor

devlinjunker commented Oct 19, 2023

Yes, the only thing missing is the settings component and different application behaviors + including the upload/download subscriptions and articles buttons.

I'm currently WIP on the Keyboard Navigation handler

Making this an alpha release sounds good to me though. I can work on getting this branch ready to merge, if someone else can take a look at updating the makefile that would be great

@SMillerDev
Copy link
Contributor

I'll update the makefile from leader after merge (unless someone else beats me to it)

@Grotax
Copy link
Member

Grotax commented Oct 20, 2023

I think the only issue is copying the js files to the right location.

And that it matches where they are linked in the app. I think all is files should land in /js since we don't have source code in that folder anymore no need for build or something.

Not sure if the admin view in the app itself is still linked to js/build but that should be an easy change.

Signed-off-by: Devlin Junker <devlin.junker@gmail.com>
[Vue Rewrite] Upmerge Branch from Master to clean up Merge Conflicts
@Grotax Grotax marked this pull request as ready for review October 24, 2023 08:19
@Grotax Grotax merged commit b60ba4a into master Oct 24, 2023
18 of 22 checks passed
@delete-merged-branch delete-merged-branch bot deleted the vue-rewrite branch October 24, 2023 09:28
Grotax added a commit that referenced this pull request Oct 24, 2023
Changed
- Major Rewrite of the Frontend with Vue JS (#748)
  For comments and suggestions for the new UI, please use this: #2388
- Set User Agent for curl in Scraper (#2380)
- Drop support for Nextcloud 26, Supported 27

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax mentioned this pull request Oct 24, 2023
Grotax added a commit that referenced this pull request Oct 25, 2023
Changed
- Major Rewrite of the Frontend with Vue JS (#748)
  For comments and suggestions for the new UI, please use this: #2388
- Set User Agent for curl in Scraper (#2380)
- Drop support for Nextcloud 26, Supported 27

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frontend is outdated