Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Steps:
1) Navigate to Structure> Taxonomy> Add Vocabulary
2) Observe the screenshot
Expected: Proper spacing is required between the text box and the text
Actual: The 'M' of 'Machine name' overlaps with the green border around the text box.
Before
After
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff.txt | 1006 bytes | lauriii |
#40 | 3097540-desktop.png | 261.61 KB | atul4drupal |
#40 | 3097540-mobile.png | 120.82 KB | atul4drupal |
#39 | interdiff_35-39.txt | 875 bytes | boulaffasae |
#39 | 3097540-39.patch | 1.22 KB | boulaffasae |
Comments
Comment #2
fly2ganesh CreditAttribution: fly2ganesh as a volunteer commentedComment #3
fly2ganesh CreditAttribution: fly2ganesh as a volunteer commentedComment #4
fly2ganesh CreditAttribution: fly2ganesh as a volunteer commentedComment #5
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedThanks for the patch @imganesh.
Review of patch #3:
diff --git a/css/src/components/form.css b/css/src/components/form.css
The CSS
src
directory isn't used anymore, since #3088805: Simplify Claro's CSS directory structure.form.pcss.css
). Instructions are in Drupal core using PostCSS for development+.form-item__suffix .machine-name-label {
I wonder if this can be generalized to
.form-item__suffix
? There will be other cases where this happens. In live websites, number fields will often have a suffix.+ margin-left: 0.75rem;
Does it need to be as much as this? I tried in the browser dev tools, and 0.25rem was enough to clear the green focus indicator.
Comment #6
fly2ganesh CreditAttribution: fly2ganesh as a volunteer commentedComment #7
fly2ganesh CreditAttribution: fly2ganesh as a volunteer commentedI agree with your point for making generalized to `.form-item__suffix`. I tested this issue with other cases and it was reproducible.
Comment #8
fly2ganesh CreditAttribution: fly2ganesh as a volunteer commentedI agree with your point for making generalized to `.form-item__suffix`. I tested this issue with other cases and it was reproducible.
Comment #9
Devipriya Rajamanickam CreditAttribution: Devipriya Rajamanickam commentedI have applied patch #7. But still I see the overlapping of 'M' with the green border around text box. I have attached screenshot also for reference.
Comment #10
ant1@imganesh: I believe you forgot to compile the CSS. Check the documentation of Front-end developer tools in core.
Comment #11
fly2ganesh CreditAttribution: fly2ganesh as a volunteer commentedThanks @antoineh, I have updated the patch with compiled CSS.
Comment #12
fly2ganesh CreditAttribution: fly2ganesh as a volunteer commentedThanks @antoineh, I have updated the patch with compiled CSS.
Comment #13
ant1Looks good.
Some comments to consider:
--space-xs
variable (equals to 0.5rem). We have these defined so why not use them :)Comment #14
ant1Addressed notes of comment #13.
Comment #15
ant1Comment #16
KondratievaS CreditAttribution: KondratievaS at Skilld commentedTested patch from #14 for mobile and desktop resolutions in Chrome, FF, Safari and IE browsers. Result is OK
Comment #17
KondratievaS CreditAttribution: KondratievaS at Skilld commentedComment #18
jungleCoding standards check must pass, see https://www.drupal.org/pift-ci-job/1614456
Comment #19
jungleMaybe #18 out of scope there.
Comment #20
apadernoWhat reported as coding standard warning for the form.pcss.css file are false positives caused from the fact the coding standard rules aren't able to handle .pcss files. (What is valid syntax for a .pcss file is not valid syntax for a .css file.)
Comment #22
xjmEmbedding the "before" and "after" screenshots in the IS for easier review.
Comment #23
xjmComment #24
xjmComment #25
ckrinaOn a design perspective the change is 👍
Comment #26
lauriiiWe should add RTL styles for this 🧐
Comment #27
boulaffasae CreditAttribution: boulaffasae as a volunteer and at Fullwave commentedComment #28
boulaffasae CreditAttribution: boulaffasae as a volunteer and at Fullwave commentedRTL styles added
Comment #29
boulaffasae CreditAttribution: boulaffasae as a volunteer and at Fullwave commentedComment #30
KondratievaS CreditAttribution: KondratievaS at Skilld commentedTested patch from #28 for mobile and desktop resolutions in Chrome, FF, Safari and IE browsers. Result is OK
Comment #31
KondratievaS CreditAttribution: KondratievaS at Skilld commentedComment #32
xjmTagging for @lauriii's review again. Thanks for the updated patch!
Comment #33
lauriiiLet's add comment here to highlight that this is for LTR https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...
We should reset the margin-left here so that both, LTR and RTL behave in the same way.
Comment #34
sauravk CreditAttribution: sauravk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #35
sauravk CreditAttribution: sauravk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #36
KondratievaS CreditAttribution: KondratievaS at Skilld commentedTested patch from #35 for mobile and desktop resolutions in Chrome, FF, Safari and IE browsers. Result is OK
Comment #37
KondratievaS CreditAttribution: KondratievaS at Skilld commentedComment #38
boulaffasae CreditAttribution: boulaffasae as a volunteer and at Fullwave commentedI think we missed the 1 point by laurii
Let's add comment here to highlight that this is for LTR https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...
Comment #39
boulaffasae CreditAttribution: boulaffasae as a volunteer and at Fullwave commentedLTR comment added
Comment #40
atul4drupal CreditAttribution: atul4drupal at Srijan | A Material+ Company commentedtested #39 which is slight improvement from #35 with respect to format standardization.
It is working as expected.
Comment #41
atul4drupal CreditAttribution: atul4drupal at Srijan | A Material+ Company commentedGoing by the practice tagging issue to be review by front-end manager
Comment #43
lauriiiSince the input element is set as display block with
max-width: 600px
, we should usemin-width: 601px here
. This is such a small change so I made the change on commit.Committed beee042 and pushed to 9.1.x. We could probably port this to 9.0.x and 8.9.x but will confirm with other committers. Thanks!
Comment #46
xjmGiven that Claro is in beta, and the bugfix is not all that disruptive, I think it is safe to backport. Applied #39 as well as @lauriii's interdiff from #43 to 9.0.x, committed, and cherry-picked to 8.9.x. Nice work!