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

Blockbase: Implement Comment Block and removed CSS #6080

Closed
wants to merge 4 commits into from

Conversation

MaggieCabrera
Copy link
Contributor

@MaggieCabrera MaggieCabrera commented Jun 10, 2022

Changes proposed in this Pull Request:

This PR implements the new comments block. I didn't add extra CSS to make it look like the old comments block and left it simple. It will be a "regression" from the previous version of the theme but since this is more of an utility block than one used for layout or design I think that's ok. Also having this on the 3.0 release of the theme makes sense if we are making a bit of a design change. The extra CSS was already causing issues on dotcom.

@beafialho I'd like your thoughts too.

Before:

Screenshot 2022-06-10 at 12 48 36

After:

Screenshot 2022-06-10 at 12 43 25

Heads up: the comment form part of the block is not yet editable via the editor, that will change in the future and we can revisit that part of the block.

Related issue(s):

Closes #5833
Closes #5847

@MaggieCabrera MaggieCabrera changed the title removed comments css and integrated comments block Blockbase: Implement Comment Block and removed CSS Jun 10, 2022
@MaggieCabrera MaggieCabrera self-assigned this Jun 10, 2022
@MaggieCabrera MaggieCabrera added this to the Blockbase 3.0 milestone Jun 10, 2022
@MaggieCabrera MaggieCabrera requested a review from a team June 10, 2022 10:47
@pbking
Copy link
Contributor

pbking commented Jun 10, 2022

Additionally: blockbase/page.html needs the same treatment.

So too could child themes take advantage: geologist, mayland-blocks, meraki, quadrat, videomaker, zoologist

Lastly quite a few non-blockbase themes could also be updated. However I expect those can be taken care of in a separate change.

@beafialho
Copy link
Collaborator

There are a few font sizes I think look better in the previous one:

  • The form's input field names "Comment", "Email", "Name" and "Website" and the "Save my name..." text next to the tick box look better in a smaller size.

Captura de ecrã 2022-06-13, às 11 12 11

However, the spacing between the form's field names and the fields looks much better in the new one 👍

@pbking
Copy link
Contributor

pbking commented Jun 13, 2022

I returned a smattering of CSS back into the theme to cover the form labels and such.
image

I also moved that markup into a template part so it could be re-used in the page template.

I tried re-using it as a drop-in in other themes. I was hopeful but that didn't work out too well for the GeoZoo brothers or Lady Quadrat. I think each of those themes will have to go through the process individually.

@MaggieCabrera
Copy link
Contributor Author

I returned a smattering of CSS back into the theme to cover the form labels and such. image

I also moved that markup into a template part so it could be re-used in the page template.

I tried re-using it as a drop-in in other themes. I was hopeful but that didn't work out too well for the GeoZoo brothers or Lady Quadrat. I think each of those themes will have to go through the process individually.

Sadly those also have a lot of custom CSS, hence why I'm trying to avoid it as much as possible!

@MaggieCabrera
Copy link
Contributor Author

I also moved that markup into a template part so it could be re-used in the page template.

I agree that we should make this bit of code reusable, but should it be a pattern instead of a template part? Are we doing this for the users or for our convenience?

@pbking
Copy link
Contributor

pbking commented Jun 14, 2022

I agree that we should make this bit of code reusable, but should it be a pattern instead of a template part? Are we doing this for the users or for our convenience?

I went back and forth. I think this shouldn't be a pattern (at least not yet) until we can leverage the forms block. Until we do achieving the target design will require some CSS and don't think we should do that for patterns but I feel more OK about it for templates. If we're keen to lose that smattering of CSS to change the font sizes it could be a pattern.

At the moment I'm doing it 100% for our convenience. Which might not be the right move but that was my motivation. :P

@MaggieCabrera
Copy link
Contributor Author

Then make it a hidden pattern?

@pbking
Copy link
Contributor

pbking commented Jun 14, 2022

There is a bit of a layout difference in the comments with the markup that we've transitioned to. Should we modify to make it layout like the previous incarnation?

Before:
image

After:
image

@MaggieCabrera
Copy link
Contributor Author

There is a bit of a layout difference in the comments with the markup that we've transitioned to. Should we modify to make it layout like the previous incarnation?

If it's something that can be done within the editor, go for it! but if we need extra CSS I'd say avoid it. I don't think visual changes are so important here since it's not something so prominent for a site's design like say a header or another block that is part of the layout of a site.

Also, extra CSS on a block that can be edited by the user can lead to weird interactions in the editor.

@pbking
Copy link
Contributor

pbking commented Jun 14, 2022

I was able to achieve the design (mostly) by using the FSE. I notice that the core/comment-date isn't picking up the font size assigned in theme.json nor what I set via FSE so there's a Gutenbug there I think. Otherwise I think the arrangement is closer to the original.

image

I've also converted it into a (hidden) pattern. The only CSS dependency is on the form so once that is gone I think we can un-hide that pattern.

@pbking
Copy link
Contributor

pbking commented Jun 14, 2022

I've opened a Gutenberg issue regarding the font-size of the date here.

@pbking
Copy link
Contributor

pbking commented Jun 16, 2022

I think this change looks dandy, especially now that this has landed.

Marking this as ready for 3.0

blockbase/block-templates/single.html Outdated Show resolved Hide resolved
pbking added a commit that referenced this pull request Jul 20, 2022
Refactor/blockbase color admin (#6043)
Moved templates from old folder location to new (#6073)
Blockbase: Implement the Button elements API (#6041)
Blockbase: Implement Comment Block and removed CSS (#6080)
Fix/migrate blockbase font self hosted (#6123)
Blockbase children: update comments block (#6153)
Blockbase: Changed the trigger to render social icons (#6079)
Blockbase: move button padding styles from ponyfill to theme.json (#5901)

Co-authored-by: Grant Kinney <[email protected]>
Co-authored-by: Jeremy Yip <[email protected]>
Co-authored-by: MaggieCabrera <[email protected]>
Co-authored-by: madhusudhand <[email protected]>
@pbking pbking mentioned this pull request Jul 20, 2022
@pbking
Copy link
Contributor

pbking commented Jul 20, 2022

Closing: Work merged in #6167

@donnapep donnapep deleted the add/comments-loop branch April 9, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blockbase: audit ponyfill for 6.0 Blockbase + block themes: comments block
3 participants