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: correct default behavior of donor wall # 3744 #3752

Merged
merged 57 commits into from
Oct 18, 2018
Merged

Conversation

ravinderk
Copy link
Collaborator

@ravinderk ravinderk commented Oct 11, 2018

Description

This PR will resolve #3744 #3745 #3758

How Has This Been Tested?

Check Acceptance Criteria of #3744 #3745

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

Important notes and Questions

  • deprecated param hide_empty (not useful, we hide empty donation by default)
  • We are showing donation completed date in donor block instead of donation creation date (Let me know if you want any change)
  • Currently we are storing meta key _give_anonymous_donation to comment and meta_key _give_has_comment to donor meta. Let me know you want to remove them but i will suggest to keep them till next two or three releases if we do not find them usefull then we will remove them.
  • We have only_comments shortcode attibutes which was allowing to show only donors which has comment and but now it will show donation with donor comments

Suggestion

  • If avatar set to true in shortcode attribute then gravatar API query will be sent (equal to number param ) which increases page load time. To improve page load performance we can render gravatar after page load with js: https://www.npmjs.com/package/gravatar-api [Created issue for that feat: load donor wall gravatars after page load #3758 ]
  • The number of SQL query reduced but since we are using give_donation_amount fn to render donation amount is the cause of increase query. We can create an alternative function where we will process donation amount in bulk but it will need update fee recovery and currency switcher plugin (Can we part of Stats API).

@kevinwhoffman
Copy link
Contributor

@ravinderk I will review this PR tomorrow and test against multisite.

I have one note so far...

deprecated param donors_per_page and renamed it to number (without backward compatibility)
Let me know if you want to add backward compatibility to this.

I think this param should stay as donors_per_page. Changing it to number makes it less clear.

@kevinwhoffman kevinwhoffman self-requested a review October 12, 2018 05:56
@kevinwhoffman
Copy link
Contributor

kevinwhoffman commented Oct 13, 2018

We should consider an option in the shortcode builder to exclude donations without comments so that we avoid awkward layouts like this where an empty comment creates too much white space.

image

To achieve this in WPBR, I checked for content at the time in which the review (or in this case, the comment) was saved. If there was no content, then I flagged the review with a taxonomy term.

While we can always add this feature later, we should consider whether it's necessary to identify these blank reviews sooner than later so that they can be queried and paginated in a performant manner.

@kevinwhoffman
Copy link
Contributor

kevinwhoffman commented Oct 16, 2018

@ravinderk I have pulled the latest from this branch and ran npm run dev but I am not seeing any gravatars for donations that I made either before or after pulling the latest changes. I only see the initials. I was seeing gravatars when previously testing this branch before this commit: c47a90c

Please confirm if you're able to see gravatars after you added the latest changes in this branch. If so, let's do a call to see what I'm missing.

@kevinwhoffman
Copy link
Contributor

kevinwhoffman commented Oct 17, 2018

Remaining Tasks

Ravinder

  • Ensure gravatars load on first-time initial page load.

Kevin

After Ravinder fixes gravatars:

  • Test donor wall on multisite and ensure it only shows donors that belong to that site.
  • Add vertical scrolling to shortcode builder.
  • Harden CSS of front-end donor wall.

Copy link
Contributor

@kevinwhoffman kevinwhoffman left a comment

Choose a reason for hiding this comment

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

@ravinderk Multisite functionality is working as expected. I have one remaining bug which needs addressed regarding donor wall behavior.

The option to Show Avatar is not working as expected. It always shows the circles with initials even when [give_donor_wall show_avatar="false"]. Please ensure avatars are hidden and we are not requesting gravatars if show_avatar="false" is in the shortcode.

It is possible for the shortcode builder to be taller than the viewport
for certain shortcodes like `[give_donor_wall]`. In these cases, the
user is unable to insert the shortcode because the button is outside the
viewport in a modal window with no way to access it.

This issue has been resolved by adding CSS that allows the modal window
to scroll vertically if it is too tall for the viewport. Shorter modal
windows will be centered vertically if they do not exceed 100% height.
Some themes such as Twenty Fifteen have overreaching styles that are
affecting the appearance of the donor wall in negative ways especially
in terms of spacing and typography.

To improve the consistency of the donor wall appearance across themes, a
selective use of `!important` has been added to prevent themes from
breaking the donor wall layout and typography.
@kevinwhoffman
Copy link
Contributor

I have hardened the CSS for better consistency in spacing and typography across themes. I used the lessens learned from WPBR theme compatibility so these should hold up fairly well.

Before/After:

image

@kevinwhoffman
Copy link
Contributor

@ravinderk Once you have fixed the Show Avatar setting that I mentioned in my review, please go ahead and merge this so we can move forward with testing for release.

@ravinderk ravinderk merged commit 9c27210 into release/2.3.0 Oct 18, 2018
@ravinderk ravinderk deleted the issue/3744 branch October 18, 2018 01:07
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.

None yet

3 participants