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

Adds option to collapse comments #820

Merged
merged 7 commits into from
Mar 31, 2020
Merged

Conversation

laurelfulford
Copy link
Contributor

@laurelfulford laurelfulford commented Mar 5, 2020

All Submissions:

Changes proposed in this Pull Request:

This PR adds an option to collapse comments to the Customizer.

The way WordPress handles comments makes this a bit tricky -- I had to do some hacky stuff to work around making sure a new comment, for example, wasn't hidden when added, or to make sure users saw the 'awaiting moderation' message.

I'm sure there are still some issues with this approach; I tried to be extensive in my testing steps, but please, hammer it has hard as you can.

Closes #804 .

How to test the changes in this Pull Request:

  1. Apply the PR and run npm run build.

  2. Navigate to Customize > Comment Options, and turn on 'Collapse Comments'.

  3. If it's not already, turn AMP mode to standard.

  4. As a logged in user, view a post with one -- the comments should not be collapsed with only one comment:

image

  1. Add a second comment. Verify that comments are not collapsed when you first add the second comment. The code is figuring out when to do this by checking the URL -- it will contain cpage or comment-page-, depending on your permalinks settings, when you add a comment:

image

  1. Remove the comment part of the URL (returning it to its normal state), and test the post again -- comments should now be collapsed:

image

  1. Test the toggle to make sure the comments open and close as expected:

image

Note: At this point you may see a double title, which is unrelated to these changes, and can be recreated on master by toggling the search. This will be fixed when #821 lands:

image

  1. Try switching to an incognito window, and add a comment as a non-logged in user; confirm that comments don't collapse after your comment is submitted, and you can see your comment awaiting moderation:

image

  1. Continue adding a few comments to the post as both new comments and replies, repeating steps 5-7 with each one, confirming things work as expected, until you have more than five top-level comments.
  2. Navigate to Dashboard > Settings > Discussion, and check the box to "Break comments into pages with"; set it to 5 top level comments:

image

  1. Navigate back to the post, and try adding a comment, as both logged in and logged out users and confirm it still works the same way (not collapsed).
  2. Expand the comments, and scroll to the bottom, then click "Previous Comments". Confirm the next page is not collapsed.
  3. Turn off AMP, and confirm the comments toggle still works (opening/closing on click).

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@laurelfulford laurelfulford added Do Not Merge! [Status] Needs Review The issue or pull request needs to be reviewed Needs documentation This feature or changes will have to be documented on newspack.blog/support after release. and removed Do Not Merge! labels Mar 5, 2020
Copy link
Contributor

@thomasguillot thomasguillot left a comment

Choose a reason for hiding this comment

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

Bellow are a couple of recommendations:

button alignment

button-alignment

The button isn't properly aligned. I'd suggest we use something like:

.comments-toggle {
    align-items: center;
    display: flex;
    justify-content: center;
}

And remove position and top from .comments-toggle svg.

button hover/focus

button-hover

There's no hover state. Maybe we could use the primary colour for the focus state?

@thomasguillot thomasguillot added [Status] Needs Changes or Feeback The issue or pull request needs action from the original creator and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Mar 26, 2020
@laurelfulford laurelfulford force-pushed the adds/804-collapsable-comments branch from 6b4522c to 6c10f3c Compare March 27, 2020 22:56
@laurelfulford
Copy link
Contributor Author

Thanks Thomas!

I've updated the toggle styles to use flexbox to vertically align the SVG.

I've also updated the focus/hover states to use the primary colour and a transparent background -- it should pick up the blue by default, or use your custom colour if it contrasts sufficiently against white. Otherwise it'll go from very dark grey to medium grey.

Just let me know if anything else needs adjustment!

@laurelfulford laurelfulford added [Status] Needs Review The issue or pull request needs to be reviewed and removed [Status] Needs Changes or Feeback The issue or pull request needs action from the original creator labels Mar 27, 2020
Copy link
Contributor

@thomasguillot thomasguillot left a comment

Choose a reason for hiding this comment

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

Thanks Thomas!

I've updated the toggle styles to use flexbox to vertically align the SVG.

I've also updated the focus/hover states to use the primary colour and a transparent background -- it should pick up the blue by default, or use your custom colour if it contrasts sufficiently against white. Otherwise it'll go from very dark grey to medium grey.

Just let me know if anything else needs adjustment!

This is much better! Thanks @laurelfulford

🚢

@thomasguillot thomasguillot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Mar 30, 2020
@laurelfulford
Copy link
Contributor Author

Thanks!

@laurelfulford laurelfulford merged commit f67ab51 into master Mar 31, 2020
@laurelfulford laurelfulford deleted the adds/804-collapsable-comments branch March 31, 2020 00:54
matticbot pushed a commit that referenced this pull request Apr 1, 2020
# [1.4.0](v1.3.0...v1.4.0) (2020-04-01)

### Bug Fixes

* correct menu name when checking for it in header ([#859](#859)) ([b560b28](b560b28))
* CSS changed by release process ([abb4bf0](abb4bf0))
* editor color palettes and secondary color by adding full, 6-digits, hex codes ([#857](#857)) ([0c800a1](0c800a1))
* make related posts styles more consistent between formats ([#761](#761)) ([b55746d](b55746d))
* make sure social links block doesn't inherit link color ([#846](#846)) ([66c927c](66c927c))
* only apply RSS icon to /feed ([#851](#851)) ([ce884ac](ce884ac))

### Features

* add a desktop slideout widget area ([#816](#816)) ([3ed6c77](3ed6c77))
* add option to collapse comments ([#820](#820)) ([f67ab51](f67ab51))
* add telephone icon to social menu ([#849](#849)) ([9812b28](9812b28))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@laurelfulford laurelfulford removed the Needs documentation This feature or changes will have to be documented on newspack.blog/support after release. label May 11, 2020
@laurelfulford
Copy link
Contributor Author

Docs have been completed on this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Collapsable Comments
3 participants