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

Fix avatar spinner size #2927

Merged
merged 1 commit into from
Jan 29, 2020
Merged

Fix avatar spinner size #2927

merged 1 commit into from
Jan 29, 2020

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Jan 28, 2020

Description

Refactored avatar component logic to be located only in Phoenix core.
Moved spinner into the avatar component.
Adjusted spinner size to match the one of the avatar to avoid layout shifting

Related Issue

Fix #2921

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@PVince81 PVince81 self-assigned this Jan 28, 2020
@PVince81 PVince81 force-pushed the fix-avatar-loading-duplicate-code branch from 6584cbf to 5a1fd3f Compare January 28, 2020 16:51
@PVince81 PVince81 changed the title Fix avatar and duplicate code Fix avatar spinner size Jan 28, 2020
@PVince81 PVince81 requested review from LukasHirt and kulmann January 28, 2020 16:52
@PVince81 PVince81 marked this pull request as ready for review January 28, 2020 16:52
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

I don't like the new loading delay behaviour. When an Avatar get's rendered, it shows nothing but the loading spinner and after 5 seconds (timeout in Avatar component) the placeholder with the colored background and one capitalized letter in the center gets displayed. The impression is that the UI is not very responsive/fast. Would be better, to show the placeholder from the start and - without a loading spinner - replace it with the avatar image as soon as it's loaded. In my opinion this doesn't need a visual loading state.

apps/files/src/components/Collaborators/Collaborator.vue Outdated Show resolved Hide resolved
apps/files/src/components/Collaborators/Collaborator.vue Outdated Show resolved Hide resolved
src/components/Avatar.vue Outdated Show resolved Hide resolved
src/phoenix.js Outdated Show resolved Hide resolved
@kulmann
Copy link
Member

kulmann commented Jan 28, 2020

The approach to have and use only one, globally available, avatar component is both good and useful. 🚀

src/components/Avatar.vue Outdated Show resolved Hide resolved
@PVince81 PVince81 mentioned this pull request Jan 29, 2020
9 tasks
@PVince81 PVince81 force-pushed the fix-avatar-loading-duplicate-code branch from 5a1fd3f to 967b1da Compare January 29, 2020 08:30
<oc-icon v-if="item.value.shareType === 1" class="uk-margin-small-right" name="group" size="large" />
<oc-icon v-else class="uk-margin-small-right" name="person" size="large" />
</template>
<oc-icon v-if="item.value.shareType === shareTypes.group" class="uk-margin-small-right" name="group" size="large" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add key to v-if, v-else

<div v-else key="collaborator-avatar-placeholder">
<oc-icon v-if="collaborator.info.share_type === '1'" class="uk-margin-small-right" name="group" size="large" />
<oc-icon v-if="$_shareType === shareTypes.group" class="uk-margin-small-right" name="group" size="large" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add key to v-if, v-else

@@ -1,12 +1,15 @@
<template>
<component :is="type" v-if="enabled">
<oc-avatar :width="width" :loading="loading" :src="avatarSource" :userName="userName" />
<oc-spinner v-if="loading" size="small" key="collaborator-avatar-spinner" :aria-label="$gettext('Loading')"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add key to v-if, v-else

@PVince81 PVince81 force-pushed the fix-avatar-loading-duplicate-code branch from 967b1da to 690fb30 Compare January 29, 2020 09:15
@PVince81
Copy link
Contributor Author

@LukasHirt I've added the requested keys, please recheck

@PVince81
Copy link
Contributor Author

and I forgot the changelog entry as this is a bugfix

Refactored avatar component logic to be located only in Phoenix core.
Moved spinner into the avatar component.
Adjusted spinner size to match the one of the avatar to avoid layout
shifting.
@PVince81 PVince81 force-pushed the fix-avatar-loading-duplicate-code branch from 690fb30 to 5df30e4 Compare January 29, 2020 09:28
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

🚀

@PVince81 PVince81 merged commit 02d43a0 into master Jan 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-avatar-loading-duplicate-code branch January 29, 2020 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oc-avatar shifts layout after loading
3 participants