Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Here's a new article about oatmeal for the Umami Demo and this issue is to get the article reviewed and added into the articles section of Umami.
Comment | File | Size | Author |
---|---|---|---|
#76 | interdiff_69-73.txt | 2.3 KB | MeenakshiG |
#74 | 2960486-73.patch | 80.55 KB | MeenakshiG |
#72 | 2960486-72.patch | 80.55 KB | MeenakshiG |
#69 | 2960486-69.patch | 80.53 KB | Vidushi Mehta |
#43 | interdiff_39-43.txt | 4.33 KB | johnny_aroza |
Comments
Comment #2
kjay CreditAttribution: kjay commentedAttached is the first version of the article ready for review, amends and turning into a patch. I'll be creating the image for this article very shortly.
Here is the content that we can use for for the article row in the default content CSV file (title,body,author,slug,image,alt,tags):
"Give your oatmeal the ultimate makeover",
give-your-oatmeal-the-ultimate-makeover.html,
"Umami",
articles/give-your-oatmeal-the-ultimate-makeover,
image-to-be-supplied.jpg,
"Alt text to be supplied",
"Vegan,Vegetarian,Oats,Breakfast,Dinner,Desert"
Comment #3
benjifisherComment #4
mjoneill CreditAttribution: mjoneill as a volunteer commentedWorking on this as part of Drupalcon Nashville.
W00t!
Comment #5
kjay CreditAttribution: kjay commentedThank you @mjoneill :)
Comment #6
kjay CreditAttribution: kjay commentedHere's a placeholder image and I'll take a properly setup one this coming week.
Thanks!
Comment #7
mjoneill CreditAttribution: mjoneill as a volunteer commentedlots of questions about what to do.
I proofed the document, and made some very minor changes. (see attached). Can you diff to see what's updated?
What now?
Comment #8
kjay CreditAttribution: kjay commented@mjoneill Thank you, the edits look good.
Next up is to work on the Umami default_content module in the Umami profile in order to turn the .txt file into a .html file and to edit the .csv file to setup the article to be imported when installing the demo.
It may help to get some pointers from previous work and there's a couple of examples of the same task going on below:
https://www.drupal.org/project/drupal/issues/2960483
And one that is probably close to complete:
https://www.drupal.org/project/drupal/issues/2939908
Comment #9
benjifisher@kjay: You can generate a link based on an issue number using the syntax
[ #2960483]
(without the extra space). For example:Comment #10
Mukeysh CreditAttribution: Mukeysh as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedAdded patch for the same. Please review.
Comment #11
Mukeysh CreditAttribution: Mukeysh as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedComment #12
Eli-TComment #13
John Cook CreditAttribution: John Cook at Creode commentedI've tested the patch. Although it produces error such as
fatal: can't open patch 'add-an-article-to-umami-oatmeal-2960486-10.patch': No such file or directory
, it still applies properly. (The error is coming from the included image data.)There are a couple of problems.
core/profiles/demo_umami/modules/demo_umami_content/default_content/articles.csv
so the importer knows about the new article.Comment #14
nigelwhiteThe h1 line "Give your oatmeal the ultimate makeover" at the top of the text file duplicates the Title of the node. Also the input filter will remove the h1 tag.
Comment #15
kjay CreditAttribution: kjay commentedHere's an image for the article that should be simple to include. We need to include an entry in the licence.txt file as well that releases this as CC4.0.
This should get the article content complete give or take any edits still needed.
Comment #16
Mukeysh CreditAttribution: Mukeysh as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedAdded patch with image added and made changes in licence.txt. Please review.
Comment #17
Mukeysh CreditAttribution: Mukeysh as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedComment #18
dmacgrue CreditAttribution: dmacgrue commentedWorking on this as part of Drupalcamp Scotland.
Comment #19
dmacgrue CreditAttribution: dmacgrue at The University of Edinburgh commentedPer comment #14, the top paragraph of text ("Give your oatmeal the ultimate makeover") still repeats the title, and should be removed.
In the current paragraph 2 I’d recommend a comma after the word well to confirm the meaning:
“Well, before you write off that boring jar of oats, you might want to take inspiration from our topping ideas that will take your oatmeal from bland to creatively delicious in just a few minutes.”
In current paragraph 3, I’d recommend the first sentence to be:
“Call it oatmeal, porridge, hot cereal or just plain oats, it is loved the world over; but recently, the humble oat is getting a funky makeover. “
In current paragraph 4 I’d suggest a comma added to the first sentence:
“It sounds great, doesn’t it?”
Under the heading: “Porridge creme brûlée”, I’d recommend adding a comma to the last paragraph:
“If you are really professional, you can use a blow torch.”
Comment #20
dmacgrue CreditAttribution: dmacgrue at The University of Edinburgh commentedComment #21
cferthorneyRerolled, and applied suggestions in #19. No interdiff due to reroll.
Comment #22
phaedrus CreditAttribution: phaedrus as a volunteer commentedWe're here at Twin Cities Drupal camp and are going to take a look at this issue.
Comment #23
thehuffman CreditAttribution: thehuffman commentedApplied patch, all changes in #19 were made. This patch adds accents to the "Crème brûlée" subhead that were missing.
Comment #24
thehuffman CreditAttribution: thehuffman commentedComment #26
phaedrus CreditAttribution: phaedrus as a volunteer commentedComment #27
phaedrus CreditAttribution: phaedrus as a volunteer commentedLooking into why the last submit failed.
Comment #28
phaedrus CreditAttribution: phaedrus as a volunteer commentedI'm not sure why the previous submit failed. I've re-done the steps that we did which were:
* Apply #21
* Tested that #21 met the changes listed in #19. It did.
* Updated creme brulee to read crème brûlée
* Created patch #28
And now I'm uploading. Hopefully, it will pass tests. If not, hopefully, the folks at the sprint can tell us what we did wrong :)
Comment #29
phaedrus CreditAttribution: phaedrus as a volunteer commented#28 is going to fail as well. Following the instructions on https://www.drupal.org/node/707484 led us to include uncommitted changes created by lando in our patch. I've re-done the previous steps again:
* Apply #21
* Tested that #21 met the changes listed in #19. It did.
* Updated creme brulee to read crème brûlée
* Created patch #29
And now I'm uploading. I'm feeling optimistic this time.
Comment #31
phaedrus CreditAttribution: phaedrus as a volunteer commentedThe order of the commit IDs in the git diff matters. It should be git diff older newer > patch
Comment #33
esod CreditAttribution: esod at Memorial Sloan Kettering Cancer Center commentedReviewing this patch at Design4Drupal in Boston.
The patch applies cleanly and the new article is added to the content. There is a message when applying the patch:
warning: 5 lines add whitespace errors.
All the whitespaces errors come from the .jpg binary and there is nothing we can do about binaries.
The content reads correctly to my eyes. Marking this issue as RTBC.
Comment #34
esod CreditAttribution: esod at Memorial Sloan Kettering Cancer Center commentedComment #35
xjmNice, thanks for working on this! Just a few small questions and nitpicks.
So, one thing I notice overall is that this article uses British English (BE) spellings and punctuations, but Drupal core and our content style guide use American English (and there is a British English translation).
I realize the content author here is a BE speaker and this probably also applies to other Umami content that's already in core. :) But since there's a British English translation of Drupal, I think probably our content should be in American English here too? Worth discussing, at least.
Some examples of this are:
The other source images have credits, but this one does not. Any particular reason?
"Gluten-free" should be hyphenated.
"Bland-looking" should be hyphenated.
"No one" should be two words, not hyphenated.
I didn't know what "caster sugar" was either. Apparently that is another British name. :)
I also wondered if the "five a day" (referencing servings of vegetables, I think) might be a little too culturally and generationally specific. (The "food pyramid" etc. taught in US schools at least has changed in recent decades.)
Aside: This sounds delicious!
"Low-sugar" should be hyphenated.
A screenshot would also be nice for reviewing the article. Thanks!
Comment #36
Eli-T@xjm thanks for taking a look,
Articles for Umami should absolutely not use British spelling etc. There is a writing style for Umami that encapsulates much of what you said in #35 in https://www.drupal.org/project/drupal/issues/2940146. There could be a better place for that, and as noted in the last OOTB initiative call, we need to make it more prominent on content issues that that exists, so that people that help out by reviewing are aware there are a set of standards to review against.
As most of the team writing articles are from the eastern side of the Atlantic, it is hard to spot terms such as caster sugar that aren't used in the US. We had some confusion with cornflour before!
Also noted:
1. The alt text is just "oatmeal, syrup". This should be improved to be more descriptive, in line with alt text used on other articles and recipes. For example, /articles/the-umami-guide-to-our-favourite-mushrooms has "A delightful selection of mushroom varieties laid out on a simple wooden plate."
2. According to https://www.bbc.com/food/caster_sugar, caster sugar is called "superfine sugar" in the US. Or we could just say "sugar", I doubt it makes that much difference to the recipe.
3. Totally agree on the 5 a day objection - that's a specific UK government drive.
Comment #37
tancAddressed xjm's items 1-7.
Addressed Eli-T's items 1-3.
Also removed .rej and .orig files mistakenly included back in #21
Screenshot of the article page added.
Comment #39
tancReroll, not sure why last patch wouldn't apply, added --binary in diff this time.
Comment #40
rachel_norfolkGoing to take a look at this today at Dev Days...
Comment #41
rachel_norfolkReview.
From xjm’s comments:
1) still a couple of listing commas missing
2) resolved
3) resolved
4) resolved
5) resolved
6) resolved
7) resolved
From Eli-T’s comments:
1) resolved
2) resolved
3) resolved
So, the only issue still remaining are the highlighted areas in the attached screenshot, where listing commas should be added.
Comment #42
rachel_norfolkAlso, as an aside, if you want to be able to check alt text on an iPad, just turn on VoiceOver! (Can’t believe it took me ten minutes to think of this! 😂)
Comment #43
johnny_aroza CreditAttribution: johnny_aroza as a volunteer and at Specbee commented@rachel_norfolk i have added the patch
Comment #44
rachel_norfolkThanks for the patch johnny but it makes changes to other pages, which are outside the scope of this issue. Can you update that patch, please, changing only the three missing commas outlined in #41?
Then I think we will be ready!
Thanks
Comment #45
johnny_aroza CreditAttribution: johnny_aroza as a volunteer and at Specbee commented@rachel i updated the patch please review
Comment #46
rachel_norfolkThanks johnny!
Latest patch incorporates all the feedback made so far and corrects the three missing listing commas. Looks RTBC to me!
Comment #48
tancTo get tests to pass I needed to diff with the —binary parameter. Rachel, did the image file get created ok in your test? I’m not near a computer to test but can look later.
Comment #49
tancJust a re-roll of #45 but with --binary to try to make tests happy.
Also confirmed patch in #45 creates the file just fine (as does this one).
Comment #51
tancNot sure why tests fail on 'trailing whitespace' within the jpeg but I'm going to mark this back to RTBC for now. Can anyone with more experience with tests on d.o explain?
Comment #52
rachel_norfolkThe image looked fine to me
Comment #53
John Cook CreditAttribution: John Cook at Creode commentedI've had a quick look at this.
A different commit (#2983996: Dedupe author in Umami) changed articles.csv, which this patch also changes. So the patch in #45/#49 needs to be re-rolled against the current 8.6.x head of core. If @johnny_bs.221B or @tanc can do this, that would be great!
I've added the "Needs reroll" tag.
Comment #54
tancReroll added
Comment #55
borisson_Back to RTBC, the trailing whitespace thing happens with images a lot, so don't worry about it.
Comment #56
Eli-TI've installed the patch in #54 against the latest 8.6.x via simplytest.me.
The patch applies, and Umami installs correctly.
The change made in #2983996 that caused the reroll to be required is still present (one Megan Collins, and she's the right one).
I've re-reviewed the article as created during install against all the points made throughout this issue and am happy everything has been addressed.
Therefore RTBC+1!
Thanks everyone :)
Comment #58
lauriii1. "Desert" - must be very dry porridge given that it's tagged with this tag 🏜🌵
2. Wondering how porridge is related to dinner? Not insisting to remove it but just want to make sure it's not there by an accident 😛
Ps. that photo is beautiful! 🤩
Comment #59
johnny_aroza CreditAttribution: johnny_aroza as a volunteer and at Specbee commentedhi @lauriii , what exactly needs to be changed.
Comment #60
borisson_1. The "desert" tag should be changed to "dessert".
2. I think the "dinner" tag should be removed as well, but not 100% sure about that.
Comment #61
johnny_aroza CreditAttribution: johnny_aroza as a volunteer and at Specbee commented@borisson_ i have made the following changes
1. The "desert" tag should be changed to "dessert". - changed
2. I think the "dinner" tag should be removed as well, but not 100% sure about that. - removed
Comment #63
Eli-TThe patch in #61 appears to be incorrectly generated, and doesn't create the give-your-oatmeal-the-ultimate-makeover.html file on application.
Agree Desert -> Dessert. I feel this mistake has been made before but can't find it right now.
However it's possible the Dinner tag is deliberate as the article briefly mentions savory porridge. Or because dessert could be a component of dinner. Will page @kjay for clarification of intent.
Comment #64
borisson_It was, and fixed in #2984001: Typo in default content (recipes)
Comment #65
kjay CreditAttribution: kjay commentedI also think we can remove the 'Dinner' tag to avoid the confusion since the article doesn't describe where porridge is served as a main.
Comment #66
borisson_As @Eli-T mentioned in #63, the patch wasn't generated correctly. This should be done again based on #54 and the changes in #60 need to be applied again.
It looks like #61 inverted interdiff and patch file names. So it's possible that renaming those files will be enough.
Comment #67
johnny_aroza CreditAttribution: johnny_aroza as a volunteer and at Specbee commented@borisson_ do u want me to rename my patch and submit again
Comment #68
borisson_Yes, what was the interdiff previously should be the patch file (at least, that's what it looks like :))
Comment #69
Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commentedAdded a full patch with a change.
Comment #70
John Cook CreditAttribution: John Cook at Creode commentedI've had a look at #69.
The patch applies cleanly against 8.7 and the tags are correct.
The section on "Super seeds" says:
This has two "and"s in quick succession. Should this be looked at and possibly use a serial commas.
Otherwise, this looks like it's nearly ready.
Comment #71
John Cook CreditAttribution: John Cook at Creode commentedSerial comma needed before the "or" in "Call it oatmeal, porridge, hot cereal or just plain oats"
"sweet, savory or even spice." – should be "spicy", and there should be another serial comma before the "or".
After talking with @kjay, the first sentence should be "For a more savory meal option, cook your oats with water, a little salt, and sprinkle with toasted seeds."
Comment #72
MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs for DrupalFit commentedDid changes as suggested by @John Cook
Comment #73
codewithlakshay CreditAttribution: codewithlakshay at OpenSense Labs commentedComment #74
MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs for DrupalFit commentedfew changes were not done so here is the correct patch
Comment #75
Eli-TThanks folks!
Could you provide an interdiff please? It makes it much easier to review when you're building on a previous patch: https://www.drupal.org/documentation/git/interdiff
Comment #76
MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs for DrupalFit commentedinterdiff of 2960486-73.patch and 2960486-69.patch
Comment #77
KondratievaS CreditAttribution: KondratievaS at Skilld commentedScreenshots were added in comments #41 and #46
Comment #78
steveparks CreditAttribution: steveparks at Convivio commentedI've tested the patch in #74 and confirm it applies cleanly to 8.6.1 and also against 8.7.x.
I've also checked back on the discussion and confirm that it has applied all changes agreed in this thread so far.
Comment #79
John Cook CreditAttribution: John Cook at Creode commentedI've checked the suggestion that I made in #71 – all have been applied.
I've also tested the patch from #74 against 8.7.x and it applied cleanly.
As this has already been check by @steveparks, I'm setting this to RTBC.
Comment #83
lauriiiCommitted 089f391 and pushed to 8.7.x. Also cherry-picked 349314c and pushed to 8.6.x. Thanks! 😋🥣