Skip to content

Add ratings block #14091

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
Merged
merged 21 commits into from
Nov 25, 2019
Merged

Add ratings block #14091

merged 21 commits into from
Nov 25, 2019

Conversation

oandregal
Copy link

@oandregal oandregal commented Nov 21, 2019 •
edited
Loading

This PR merges the existing rating block into Jetpack.

  • Fix styles in front-end.
  • Add PHPDoc.
  • Remove spiciness and priciness blocks from server rendering.
  • Remove spiciness and priciness blocks from client rendering.
  • Rename folder and block to rating-star.
  • Remove SVG and CSS related to spiciness and priciness.
  • How to include the missing files in D35777-code

@oandregal oandregal requested a review from a team November 21, 2019 18:23
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello nosolosw! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D35777-code before merging this PR. Thank you!

@oandregal oandregal requested a review from jeherve November 21, 2019 18:24
@jetpackbot
Copy link
Collaborator

jetpackbot commented Nov 21, 2019 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 14ee7e3

@oandregal
Copy link
Author

oandregal commented Nov 21, 2019 •
edited
Loading

I'm a bit confused as to why front-end styles aren't picked up (front-end should look like in the editor). I'll resume this work tomorrow with a fresh mind.

@matticbot
Copy link
Contributor

nosolosw, Your synced wpcom patch D35777-code has been updated.

@jeherve jeherve added [Type] Feature Request [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Star Rating labels Nov 22, 2019
@jeherve jeherve added this to the 8.0 milestone Nov 22, 2019
@matticbot
Copy link
Contributor

nosolosw, Your synced wpcom patch D35777-code has been updated.

@matticbot
Copy link
Contributor

nosolosw, Your synced wpcom patch D35777-code has been updated.

@oandregal oandregal added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Nov 22, 2019
@oandregal
Copy link
Author

oandregal commented Nov 22, 2019 •
edited
Loading

This is ready to review.

Note that I've deleted all references to ratings-spiciness and ratings-priciness except in two cases:

I've done this because I hope to be able to load or depend on those files in the WordPress.com code for spiciness and priciness. If we prefer just duplicate the code there I'm happy to remove these as well.

Edit: note that in cc9a951 I removed these traces as well. We go for independent blocks that don't share any code.

@matticbot
Copy link
Contributor

nosolosw, Your synced wpcom patch D35777-code has been updated.

simison
simison reviewed Nov 22, 2019
View reviewed changes
@oandregal oandregal added the [Status] Needs Review This PR is ready for review. label Nov 25, 2019
@oandregal
Copy link
Author

In reviewing D35777-code, I've noticed that a few files are missing:

  • The extensions/rating-star folder, that contains the PHP files that register the block.
  • The _inc/rating-star folder that contains the view files.

Should I add them manually after this lands (so they don't get overriden) or is there any other thing I should do to register the block?

@jeherve
Copy link
Member

jeherve commented Nov 25, 2019

Could you elaborate what's the issue? I've tested and it seems to work fine.

The stars are aligned to the left of the preview window, with no padding or anything. I would expect them to be aligned to the center.

@jeherve
Copy link
Member

jeherve commented Nov 25, 2019

I've noticed that a few files are missing:

  • The extensions/rating-star folder, that contains the PHP files that register the block.
  • The _inc/rating-star folder that contains the view files.

You will indeed need to add the extensions/rating-star and all its components manually (php, js, scss, everything from that directory must be committed).
Once you svn add those files, TeamCity will be able to build a new bundle for you. You consequently don't need to commit the _inc/blocks/rating-star directory since TeamCity takes care of creating and adding the built files to your diff for you, and you can then commit everything from the diff the bot will have updated.

Let's do this together once you're ready if you'd like!

jeherve
jeherve reviewed Nov 25, 2019
View reviewed changes
@jeherve jeherve mentioned this pull request Nov 25, 2019
Closed
@oandregal
Copy link
Author

The stars are aligned to the left of the preview window, with no padding or anything. I would expect them to be aligned to the center.

Ah, ok. So, alignment is a user choice and by default they're left-aligned. I agree a bit more of padding could make them stand out a bit more (and also make the symbols bigger) but not sure how we would write CSS that targets the preview without compromising the other scenarios. I'd say this could be a future improvement if we wanted to.

jeherve
jeherve previously approved these changes Nov 25, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This is looking good. I think I'll merge this now, it's in a good state and we can start testing it tomorrow with the Beta!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Nov 25, 2019
@jeherve
Copy link
Member

jeherve commented Nov 25, 2019

Just pushed 14ee7e3 to fix some comments. Will merge when the tests complete.

@jeherve jeherve merged commit 88f8689 into master Nov 25, 2019
@jeherve jeherve deleted the add/ratings branch November 25, 2019 20:55
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 25, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
@jeherve
Changelog: add #14091
e3c6c15
jeherve added a commit that referenced this pull request Nov 25, 2019
@jeherve
8.0 Release: running changelog (#13964)
807ab89 
* 8.0 Release: running changelog

* Changelog: add #13921

* Changelog: add #13980

* Changelog: add #13905

* Changelog: add #13971

* Changelog: add #13984

* Changelog: add #14009

* Changelog: add #13620

* Remove things that will ship in 7.9.1

* Changelog: add 7.9.1 release (#14044)

* Changelog: add base for 7.9.1 release

* Update release date and post link

* Changelog: add #14066

* Update changelog for 7.9.1

* Changelog: add #13405

* Changelog: add #13841

* Changelog: add #13924

* Changelog: add #13986

* Changelog: add #14010, #14028, #14053, #14055.

* Changelog: add #14054

* Changelog: add #14031

* Changelog: add #14039

* Changelog: add #14050

* Changelog: add #14070

* Changelog: add #14082

* Changelog: add #14084

* Changelog: add #14111

* Changelog: add #13961

* Changelog: add #14047

* Changelog: add #14091

* Changelog: add #14108

* Changelog: add #14121
@simison
Copy link
Member

simison commented Nov 26, 2019

Please remember to commit D35777-code so that files are back to sync.

Nice work on this, @nosolosw ! 🌟

@jeherve jeherve mentioned this pull request Nov 26, 2019
Open
@oandregal
Copy link
Author

D35777-code has also been deployed and this is already on WordPress.com

akirk
akirk reviewed Dec 6, 2019
View reviewed changes
@jeherve jeherve mentioned this pull request Dec 6, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simison simison simison left review comments

@akirk akirk akirk left review comments

@jeherve jeherve jeherve approved these changes

@jasmussen jasmussen jasmussen left review comments

Assignees

@oandregal oandregal

Labels
[Block] Star Rating [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Feature Request
Projects
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.
7 participants
@oandregal @matticbot @jetpackbot @simison @jeherve @akirk @jasmussen