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

Add user-status to Avatar component #1259

Merged
merged 6 commits into from
Aug 18, 2020
Merged

Add user-status to Avatar component #1259

merged 6 commits into from
Aug 18, 2020

Conversation

georgehrke
Copy link
Contributor

@georgehrke georgehrke commented Aug 5, 2020 •
edited
Loading

  • Icons need attribution (Material UI)
  • add white border around icons
61D15EEB-050C-4EB4-B565-CAA7DBC3B8D6 170CA5A4-4621-4ED2-A833-027CFF03A2F8
76349057-CF32-405F-9DFA-D66C3197A138

@georgehrke georgehrke added the 2. developing Work in progress label Aug 5, 2020
@georgehrke georgehrke mentioned this pull request Aug 5, 2020
Closed
8 tasks
@georgehrke georgehrke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 6, 2020
@georgehrke
Copy link
Contributor Author

georgehrke commented Aug 6, 2020 •
edited
Loading

There are two things which i would like to add later in a follow-up PR:

  • Periodically refresh the status
  • Have some kind of central store for statuses, so we don't fetch it dozens of times for the same user. This is especially important for Talk, where there are many Avatars of the same user.
  • Migrate ContactsMenu from PopoverMenu to Actions.

@georgehrke georgehrke marked this pull request as ready for review August 6, 2020 09:46
@skjnldsv skjnldsv added the feature: avatar Related to the avatar component label Aug 11, 2020
@ChristophWurst
Copy link
Contributor

What is the expected handling of Nextcloud 19 and older? Send off the request anyway and catch the missing route error?

@ChristophWurst
Copy link
Contributor

Periodically refresh the status

https://github.com/nextcloud/nextcloud-push if we have that one day

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 14, 2020
@georgehrke
Copy link
Contributor Author

What is the expected handling of Nextcloud 19 and older?
Send off the request anyway and catch the missing route error?

We could check the ocs capabilities API, but that adds yet another request.

What is the best way to store the status among multiple instances of Avatar?
I would avoid adding vuex just for this.

@georgehrke georgehrke added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Aug 14, 2020
@georgehrke
Copy link
Contributor Author

I moved the status fetching to a mixin, so we can also use it in the AvatarSelectOption.vue component.
(see screenshot in original post)

@georgehrke georgehrke mentioned this pull request Aug 14, 2020
Merged
@georgehrke
Copy link
Contributor Author

Squashed and rebased

nickvergessen
nickvergessen approved these changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Works (with the mentioned follow up issues)

@nickvergessen
Copy link
Contributor

Pushed a PR to allow to control the status displaying on the mention chip

@ChristophWurst
Copy link
Contributor

We could check the ocs capabilities API, but that adds yet another request.

OC.getCapabilities or similar?

@georgehrke
Copy link
Contributor Author

We could check the ocs capabilities API, but that adds yet another request.

OC.getCapabilities or similar?

Forgot we have that. Thx!
I added it to the PR.

@juliushaertl
Copy link
Contributor

There is also https://github.com/nextcloud/nextcloud-capabilities/

ChristophWurst
ChristophWurst requested changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Let's use #1259 (comment) :)

@rullzer
Copy link
Contributor

rullzer commented Aug 18, 2020

sooo merge?

@skjnldsv
Copy link
Contributor

sooo merge?

#1259 (review)

@georgehrke georgehrke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 18, 2020
@georgehrke
Copy link
Contributor Author

@rullzer Fixed, please hit merge 😉

@georgehrke georgehrke merged commit 6042058 into master Aug 18, 2020
@georgehrke georgehrke deleted the feature/20931/status branch August 18, 2020 07:35
georgehrke added a commit that referenced this pull request Aug 18, 2020
@georgehrke
Fix linting error introduced by #1259
c0a7387 
Signed-off-by: Georg Ehrke <[email protected]>
skjnldsv added a commit that referenced this pull request Aug 18, 2020
@skjnldsv
Merge pull request #1304 from nextcloud/bugfix/noid/status-linting
a9d5c9f 
Fix linting error introduced by #1259
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@juliushaertl juliushaertl juliushaertl left review comments

@skjnldsv skjnldsv skjnldsv approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@rullzer rullzer Awaiting requested review from rullzer

Assignees
No one assigned
Labels
3. to review Waiting for reviews feature: avatar Related to the avatar component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@georgehrke @ChristophWurst @nickvergessen @juliushaertl @rullzer @skjnldsv