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

Move conversation menu options to a dialog #4576

Merged
merged 2 commits into from
Nov 30, 2020
Merged

Move conversation menu options to a dialog #4576

merged 2 commits into from
Nov 30, 2020

Conversation

PVince81
Copy link
Member

@PVince81 PVince81 commented Nov 11, 2020 •
edited
Loading

For #4007

Current state

See #4576 (comment)

Old state from first attempt

image

image

Tasks

  • move sharing settings
  • add/copy "copy link" also to the settings tab
  • move lobby settings to settings tab
  • move SIP options to settings tab
  • remove lobby + locking panels while in call
  • disable locking checkbox while in a call => will do separately in Feature/noid/conversation settings error handling #4671
  • improve design
  • add more hints

Tests

  • retest permissions and switches
    • owner/moderator
    • guest moderator
    • non-moderator

@PVince81 PVince81 force-pushed the feature/4007/conversation-menu-to-sidebar branch 2 times, most recently from 5a17b04 to 3731483 Compare November 11, 2020 15:10
@PVince81 PVince81 mentioned this pull request Nov 11, 2020
Closed
4 tasks
@PVince81 PVince81 self-assigned this Nov 11, 2020
@PVince81
Copy link
Member Author

See screenshot above for the current state.

  • I've used the "Action*" components as they look nice and align well, I hope I can keep them as I don't want to juggle with "regular" elements.

  • It seems it's getting crowded quickly, should we move the sections to separate tabs ?

@nextcloud/designers-talk

@PVince81 PVince81 added this to the 💚 Next Major (21) milestone Nov 11, 2020
marcoambrosini
marcoambrosini reviewed Nov 11, 2020
View reviewed changes
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

I'd suggest using the AppSettingsDialog vue component for conversation settings too, and get rid of the sidebar settings tab!

@PVince81 PVince81 changed the title Add "more settings" menu item to open sidebar settings Move conversation menu options to the sidebar Nov 11, 2020
@PVince81 PVince81 force-pushed the feature/4007/conversation-menu-to-sidebar branch from 3731483 to 6d890b8 Compare November 12, 2020 14:42
@PVince81
Copy link
Member Author

PVince81 commented Nov 12, 2020 •
edited
Loading

Now it's in a dialog:
image

@PVince81
Copy link
Member Author

it seems something is wrong with the app settings component, it doesn't scroll properly:
image

@ma12-co I didn't add any CSS, shouldn't the dialog work out of the box ?

@PVince81
Copy link
Member Author

@ma12-co can you have a look at the scrollbar thing ?

@PVince81 PVince81 mentioned this pull request Nov 16, 2020
Merged
@PVince81
Copy link
Member Author

here's a fix for the scrollbar issue: nextcloud-libraries/nextcloud-vue#1573

@PVince81 PVince81 changed the title Move conversation menu options to the sidebar Move conversation menu options to a dialog Nov 17, 2020
@PVince81 PVince81 force-pushed the feature/4007/conversation-menu-to-sidebar branch from ac36fe3 to 95b6a98 Compare November 17, 2020 11:28
@PVince81
Copy link
Member Author

@ma12-co as discussed, I've added some text hints.

As advised by @nickvergessen I've merged the SIP section into the moderation one.

Now it seems that with only two sections, the left navigation bar is redundant so maybe we can disable it for this dialog.

Let me know what you think.

@nickvergessen
Copy link
Member

Mind to update the screenshot in the first post?

@PVince81 PVince81 marked this pull request as ready for review November 17, 2020 12:50
@PVince81
Copy link
Member Author

screenshots updated.

@PVince81
Copy link
Member Author

PVince81 commented Nov 17, 2020 •
edited
Loading

  • try renaming the moderation section to "Meeting settings"

@PVince81
Copy link
Member Author

PVince81 commented Nov 20, 2020 •
edited
Loading

I've removed all the Action* and replaced them with plain old HTML elements, but following a similar structure.

New WIP state:

image

image

  • fix margins/paddings
  • fix DatePicker visually broken
  • submit button a bit shifted

@PVince81
Copy link
Member Author

PVince81 commented Nov 23, 2020 •
edited
Loading

@PVince81 PVince81 mentioned this pull request Nov 25, 2020
Closed
2 tasks
@PVince81 PVince81 force-pushed the feature/4007/conversation-menu-to-sidebar branch from 176a099 to 7573562 Compare November 25, 2020 16:24
@PVince81
Copy link
Member Author

Rebased.

And I've split error handling and special cases which did not exist before to a follow up PR: #4671

I'd like to already merge this PR here as it's the same functionality as before but moved to the dialog.

@PVince81
Copy link
Member Author

hmmm, still requires a release of nextcloud-vue for nextcloud-libraries/nextcloud-vue#1573

when I tested I had nextcloud-vue master linked

@PVince81
Copy link
Member Author

nextcloud-vue update here #4680, it will take a bit longer as we need to adjust some components affected by the update

@PVince81 PVince81 mentioned this pull request Nov 27, 2020
Merged
8 tasks
@nickvergessen
Copy link
Member

vue update is in

@PVince81 PVince81 force-pushed the feature/4007/conversation-menu-to-sidebar branch from 7573562 to a6f9dce Compare November 27, 2020 14:30
@PVince81
Add "more settings" menu item to open sidebar settings
959abc1 
Created a new dialog for conversation settings.
It includes the share link settings (aka guest access), the
moderation settings and the SIP settings.

Added notifications after success/error when saving.
Added field disabling logic while saving which provides some visual
feedback.

Refactored various "conversation" computed properties to all use the
same dummy conversation object when the token was not found.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Replace Action components with plain HTML inputs
828af67 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81 PVince81 force-pushed the feature/4007/conversation-menu-to-sidebar branch from a6f9dce to 828af67 Compare November 27, 2020 14:52
@PVince81
Copy link
Member Author

Rebased and did a quick retest.

One difference from before the rebase is that now the SIP checkbox has a condition for displaying.
To make it appear you need to fill in all the fields in the SIP section in the admin settings for Talk.

@PVince81
Copy link
Member Author

ready to review :-)

@PVince81
Copy link
Member Author

restarted CI

@marcoambrosini marcoambrosini merged commit de7672d into master Nov 30, 2020
@marcoambrosini marcoambrosini deleted the feature/4007/conversation-menu-to-sidebar branch November 30, 2020 14:56
@nickvergessen
Copy link
Member

Merged too early...

The SIP changes were not merged and they need a backport

@PVince81
Copy link
Member Author

I'm fine reverting and rebasing later.

seems there's only a conflict in top bar markup, this line https://github.com/nextcloud/spreed/pull/4689/files#diff-841f2c9a1004d8fafe1e1f6fe44412b062860ea277a4ddc071068f7f2a162e7dR146

@nickvergessen
Copy link
Member

"only", its not an ActioItem anymore :P

@PVince81 PVince81 mentioned this pull request Nov 30, 2020
Closed
@nickvergessen nickvergessen mentioned this pull request Nov 30, 2020
Merged
@PVince81
Copy link
Member Author

here we go, the revert PR: #4701

@PVince81 PVince81 mentioned this pull request Dec 14, 2020
Merged
50 tasks
@nickvergessen nickvergessen mentioned this pull request Jan 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcoambrosini marcoambrosini marcoambrosini approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@danxuliu danxuliu Awaiting requested review from danxuliu

Assignees

@PVince81 PVince81

Labels
3. to review enhancement feature: chat 💬 Chat and system messages
Projects
None yet
Milestone
v11.0.0-alpha.1
Development

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