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.

Sign up for GitHub

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

Jump to bottom

Adjust Files sidebar tab to changes in server #4291

Merged
merged 3 commits into from
Nov 11, 2020
Merged

Adjust Files sidebar tab to changes in server #4291

merged 3 commits into from
Nov 11, 2020

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Oct 5, 2020

@danxuliu danxuliu added 2. developing feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents labels Oct 5, 2020
@danxuliu danxuliu added this to the 💚 Next Major (21) milestone Oct 5, 2020
@danxuliu danxuliu mentioned this pull request Oct 5, 2020
Merged
@skjnldsv skjnldsv mentioned this pull request Oct 14, 2020
Closed
@skjnldsv skjnldsv requested a review from PVince81 October 14, 2020 16:26
@PVince81
Copy link
Member

PVince81 commented Oct 15, 2020 •
edited
Loading

I'd suggest rebasing against spreed master as this branch doesn't run with the latest server master (PHP errors).
After a local rebase I can run it.

image

=> seems it's looking for an element id that doesn't exist in sidebar mode, might need to set the id "conversaion_$token" on the scroll container

Edit: I've submitted a fix here #4531

@danxuliu
Copy link
Member Author

Indeed, it needs a rebase. That is one of the reasons why it is a draft not ready for review yet :-P

@PVince81
Copy link
Member

yeah I know, no worries. :-)

just needed this branch to check whether sidebar mode still works in my PR #4363

@PVince81
Copy link
Member

PVince81 commented Oct 15, 2020 •
edited
Loading

  • BUG: switching current file while a chat is open doesn't switch chat tab context. maybe some event handling missing

I know you're working on it, just wanted to mention it as I first thought it was a bug on master but isn't ^

This was referenced Oct 15, 2020
Merged
Merged
@PVince81
Copy link
Member

any update ? need help ?

it's now the third time I need to test some changes with the sidebar :-/

@PVince81 PVince81 mentioned this pull request Oct 28, 2020
Merged
9 tasks
@PVince81
Copy link
Member

PVince81 commented Oct 28, 2020 •
edited
Loading

  • BUG: upload not starting after selecting a file, also no dialog appears

This was referenced Oct 28, 2020
Merged
Merged
@PVince81 PVince81 mentioned this pull request Nov 4, 2020
Merged
@PVince81 PVince81 force-pushed the adjust-files-sidebar-tab-to-changes-in-server branch from b1d3aee to 9ab3c5c Compare November 4, 2020 16:33
@PVince81
Copy link
Member

PVince81 commented Nov 4, 2020

@danxuliu I took the liberty of rebasing this and fixing the above issues:

  • upload dialog is now included and works, so you can attach files
  • switching the file info now properly switches the view

was there anything else to work on to finish this ? (a checkbox list would be nice)

If you intended to do bigger cleanups I'd still be in favor of merging this (if the state is acceptable) at least to fix the regression and not block other people. The cleanup could be addressed separately.

@danxuliu danxuliu mentioned this pull request Nov 5, 2020
Merged
@PVince81 PVince81 mentioned this pull request Nov 5, 2020
Closed
@danxuliu danxuliu force-pushed the adjust-files-sidebar-tab-to-changes-in-server branch from 9ab3c5c to c1d018e Compare November 9, 2020 21:40
@danxuliu
Remove unused code
33492ce 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Adjust Files sidebar tab to changes in server
0029583 
When a tab is registered in the Files sidebar a component is no longer
provided. Instead an object with some attributes that describe
the tab (like id or icon) and some callbacks for lifecycle events is
expected now.

Essentially the AppSidebarTab component is now created by the Sidebar
itself instead of being given it. However, as the component needed some
adjustments in its CSS style that needs to be forced when the child
component is mounted.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Remove no longer needed Vue adapter
3a8de08 
With the changes in the Files app sidebar API the Chat tab can be
directly used in the OCA.Files.Sidebar.Tab object without having to use
a special Vue component as a bridge.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the adjust-files-sidebar-tab-to-changes-in-server branch from c1d018e to 3a8de08 Compare November 9, 2020 21:41
@danxuliu
Copy link
Member Author

danxuliu commented Nov 9, 2020

Chat tab in Files sidebar should work again just like before. Note that there are some issues unrelated to the changes here:

@danxuliu danxuliu marked this pull request as ready for review November 9, 2020 21:42
@PVince81
Copy link
Member

PVince81 commented Nov 10, 2020 •
edited
Loading

@danxuliu let me know if/when you need another review ok, saw the label

@PVince81
Copy link
Member

Calls are shown in grid mode instead of speaker mode

@danxuliu let's raise tickets for the remaining issues, I can also help out with some like the remembering one (you could tag as regression)

PVince81
PVince81 approved these changes Nov 10, 2020
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 please raise an issue for the style hack if not solvable

I find it interesting that the PR size has reduced over time

src/mainFilesSidebarLoader.js
}

// Dirty hack to force the style on parent component
const tabChat = document.querySelector('#tab-chat')
Copy link
Member

Choose a reason for hiding this comment

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

no proper way ? can we instead set a class on the parent that we can style from here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I am not mistaken there have been some changes in the Files app sidebar related to this, I would need to check again.

@PVince81 PVince81 mentioned this pull request Nov 10, 2020
Closed
@PVince81
Copy link
Member

I've pushed this PR for keeping in non-grid mode when in sidebar: #4573

@PVince81 PVince81 mentioned this pull request Nov 10, 2020
Closed
@PVince81
Copy link
Member

Raised #4574 for sidebar call regressions which I noticed, that are unrelated to this PR. Those issues likely have sneaked in as I didn't always test with the sidebar (as it was broken)

@nickvergessen
Copy link
Member

So, merge?

@PVince81 PVince81 merged commit 66d767c into master Nov 11, 2020
@PVince81 PVince81 deleted the adjust-files-sidebar-tab-to-changes-in-server branch November 11, 2020 08:58
@matiasdelellis matiasdelellis mentioned this pull request Mar 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@PVince81 PVince81 PVince81 approved these changes

Assignees
No one assigned
Labels
3. to review feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents regression
Projects
None yet
Milestone
v11.0.0-alpha.1
Development

Successfully merging this pull request may close these issues.
3 participants
@danxuliu @PVince81 @nickvergessen