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] BugFix (Feed Actions) + Warning Message + subscribe_to URL parameter #2360

Merged

Conversation

devlinjunker
Copy link
Contributor

@devlinjunker devlinjunker commented Sep 26, 2023

Related to #748

Follows #2354

🗒️ Summary

  • Fix Sidebar Feed Actions Menu (broken from rebase)
  • add warning message generated with PHP
  • check for subcribe_to parameter in sidebar to open popover and in popover to pre-populate input

✅ Checklist

- [ ] Changelog entry added for all important changes. (Can we add the skip-changelog label to prevent the check for changelog? )

📷 Visual

Feed Actions Menu:
Screenshot 2023-09-26 at 11 58 45 AM

Warning Message:
Screenshot 2023-09-26 at 11 58 51 AM

subscribe_to URL Param:
subcribe_to

➡️ Up Next

MVP:

  • Share Menu
  • Audio/Video Feed Items
  • Mobile friendly layout (since we are no longer displaying the feed item below the row)
  • Update Documentation for Front End
    • Developer Tips/Organization
    • Remove Frontend/JS Plugin Documentation
    • Front End Unit Tests
  • Settings in Sidebar
    • Keyboard Navigation
    • Mark on Scroll
    • Reverse Order (Oldest to Newest)
    • Upload/Download Subscriptions/Articles
  • Error Messages (on backend request failure)
  • Fix Explore Page

Following Rewrite Release Improvements:

  • addArticleAction Plugin API
  • 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)
  • Additional Views (Compact/Expanded etc)

…rameter

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

Grotax commented Sep 27, 2023

Hmm the frontend test failed, did you check why?

@devlinjunker
Copy link
Contributor Author

devlinjunker commented Sep 27, 2023

@Grotax oh yeah. I can fix that quickly today. Just because it’s checking for the subscribe_to url param in the test but I didn’t set up any mocks for it.

For the static analysis failure, are you going to create a PR against the master branch to fix that? If so, I can do an upmerge to fix that once you do

@Grotax
Copy link
Member

Grotax commented Sep 27, 2023

I didn't check in detail but based on the error message and since we don't have that on master I think it comes from the comment you added in that php file.

I can check before I merge this should be easy to fix

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

@Grotax I'm not sure it comes from the comment, I see this in the logs of the failing job:

 ------ --------------------------------------------------------------- 
  Line   Db/ItemMapperV2.php                                            
 ------ --------------------------------------------------------------- 
  204    Fetching class constant ASSOCIATIVE of deprecated class        
         Doctrine\DBAL\FetchMode:                                       
         Use the dedicated fetch*() methods for the desired fetch mode  
         instead.                                                       
  230    Fetching class constant COLUMN of deprecated class             
         Doctrine\DBAL\FetchMode:                                       
         Use the dedicated fetch*() methods for the desired fetch mode  
         instead. 

I think this has to do with the dependabot update that was recently merged: #2362

I also see the error in the release cut PR: #2359

@Grotax
Copy link
Member

Grotax commented Sep 28, 2023

Ah now I see, yea I'm not sure if we can change the code.
dbal is actually just a dev dependency when installed we use the version of the server.
And they use an older version https://github.com/nextcloud/3rdparty/blob/master/composer.json

But the phpcs complains in this PR could be fixed.

@devlinjunker
Copy link
Contributor Author

ah, I see, I'll clean up that phpcs failure today

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

Grotax commented Oct 1, 2023

Tested and works after clean rebuild of my dev environment, thanks :)

@Grotax Grotax merged commit e6f5edb into nextcloud:vue-rewrite Oct 1, 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.

2 participants