Comments

Webbeh created an issue. See original summary.

Webbeh’s picture

Status: Active » Needs review
StatusFileSize
new3.95 KB

Patch applied to bring README.MD into best practices.

Webbeh’s picture

Assigned: Webbeh » Unassigned
Anonymous’s picture

Assigned: Unassigned »
ricksta’s picture

Status: Needs review » Reviewed & tested by the community

Tested and checks out fine.

ricksta’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new8.15 KB

I actually found a few typos, so here's a new patch and interdiff.

ricksta’s picture

And the patch.

volkswagenchick’s picture

Assigned: » Unassigned
Issue tags: +NorthAmerica2021, +Novice, +Documentation

I am asking that this issue be reserved for a novice during DrupalCon. Thanks

Adding NorthAmerica2021 tag for visbility.

DrupalCon NA is April 12-16 with a focus on EOOTB on Wednesday, April 14. This is a layout builder adjacent extension and is used in addition to core.
Thanks

marcusvsouza’s picture

The patch in comment #7 does not apply, so i did a re-roll of the patch and add some information to match with the standards

volkswagenchick’s picture

Status: Needs review » Needs work
+++ b/README.md
@@ -13,15 +21,25 @@ Let’s say you want to provide the user with the ability to write text segments
+SystemLayout Builder ¶

There is an empty space at the end of line 32
There needs to be hard line breaks at 80 characters
Two lines before headings.

I suggest working off the patch in comment 2. Thanks. Moving to needs work

ankithashetty’s picture

Status: Needs work » Needs review
StatusFileSize
new3.95 KB
new1.31 KB

As suggested in #10, updated the patch in #2 to fix minor typo issues. Attached an interdiff file as well.

And looks like the patch & interdiff submitted in #6 and #7 have exchanged their file extension! So they are not valid files.

Thank you!

marcusvsouza’s picture

Status: Needs review » Reviewed & tested by the community

The patch in comment #11 correctly apply the standards and fix the typo off comment #2.

johnwebdev’s picture

Status: Reviewed & tested by the community » Needs work

Hello!

Thanks for the patch and work so far!

Looks like there is some bad encoding in here: -Let’s, let’s that needs to be resolved.

manthan.chauhan’s picture

Status: Needs work » Needs review
StatusFileSize
new3.95 KB

Hello @johnwebdev,

Corrected special characters, kindly review patch.

Thanks

hmendes’s picture

Status: Needs review » Reviewed & tested by the community

The patch look great now. Changing this to RTBC.

johnwebdev’s picture

Status: Reviewed & tested by the community » Needs work

Still some occurrences of Let’s for me.

hmendes’s picture

Hello, The occurrences of Let’s missing are in the parts the are being removed... is that a problem?
The current code is writing lets with the ’ character instead of ' That's why the problem is happening.

volkswagenchick’s picture

Line 55 - let's not use a node alias.
Let's use https://www.drupal.org/node/1897420/ instead. That was it is drupal version agnostic.

hmendes’s picture

Status: Needs work » Needs review
StatusFileSize
new3.91 KB
new416 bytes

Adding the suggestion from #18.

dandeleon03’s picture

StatusFileSize
new302.43 KB

Hi @hmendes,

Checked and confirmed that node alias is no longer used.

Kindly see screenshot.

Thanks.

dandeleon03’s picture

Status: Needs review » Reviewed & tested by the community
johnzzon’s picture

Status: Reviewed & tested by the community » Needs work

-Let’s say you want to provide

There are still 2 occurrences of "Let’s". Please write the text in UTF-8 encoding.

EDIT: I just read that they're in the removed lines as you said. Can anyone confirm that it applies cleanly? If so, it shouldn't be an issue.

Webbeh’s picture

Status: Needs work » Reviewed & tested by the community

EDIT: I just read that they're in the removed lines as you said. Can anyone confirm that it applies cleanly? If so, it shouldn't be an issue.

Confirming patch #19 resolves this - returning to RTBC.

  • johnzzon committed 38906d6 on 8.x-1.x
    Issue #3112521 by hmendes, ricksta, marcusvsouza, ankithashetty, Webbeh...
johnzzon’s picture

Patch merged! Thanks for everyone's participation, it's greatly appreciated!

johnzzon’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.