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

Check for existing of avatar_urls array before trying to return the avatar img part of user autocomplete fragment #18259

Merged
merged 3 commits into from
Jan 16, 2020

Conversation

glendaviesnz
Copy link
Contributor

Description

Currently the user mention autocomplete fails if the site is returning false for get_option( 'show_avatars' ).

Fixes #18258

How has this been tested?

Tested manually by toggling show_avatars
Added simple unit test to ensure return of user details fragment doesn't fail if avatars array not set

Screenshots

Before:
user-auto-complete

After:

after-mention-fix

Types of changes

Made the packages/editor/src/components/autocompleters/user.js getOptionLabel method defensive to check for existence of the avatar url before trying to set img tag

Checklist:

  • [ X ] My code is tested.
  • [ X ] My code follows the WordPress code style.

@glendaviesnz glendaviesnz changed the title Check for existing of avatar_urls array before trying to return the avatar img part of user autocomplete fragmen Check for existing of avatar_urls array before trying to return the avatar img part of user autocomplete fragment Nov 4, 2019
@kwight
Copy link

kwight commented Nov 6, 2019

If I understand correctly, show_avatars is related to showing avatars for commenters on the front-end of the site – I'm not sure we have the same expectation when using autocomplete. Another option we have is to treat the autocomplete the same as the username in the top right corner; when show_avatars is off, a small generic icon is still used:

Screen Shot 2019-11-06 at 1 10 43 PM

@glendaviesnz
Copy link
Contributor Author

Another option we have is to treat the autocomplete the same as the username in the top right corner; when show_avatars is off, a small generic icon is still used:

I have modified the PR to use this approach:

Screen Shot 2019-11-11 at 1 55 22 PM

@glendaviesnz
Copy link
Contributor Author

@talldan, if you could cast your eyes over this when you get a chance that would be great. The immediate need for this fix has gone, as the issue with missing avatars we had was resolved, but would be good to get this merged in to prevent the same issue if there is ever a regression with the avatars.

@glendaviesnz
Copy link
Contributor Author

@talldan, @youknowriad - would be good to get this one either merged or closed if you have a chance to look at it in the near future. Thanks.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I haven't tested this but code-wise this is looking good to me.

@glendaviesnz
Copy link
Contributor Author

Rebased and did some more manually testing of this, all still works as advertised so will go ahead and merge.

@glendaviesnz glendaviesnz merged commit 9c5c691 into WordPress:master Jan 16, 2020
@glendaviesnz glendaviesnz deleted the fix/missing-avatar-bug branch January 16, 2020 20:32
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
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.

User mention autocompletion fails if avatar_urls array is not set
4 participants