See: #1995272: [Meta] Refactor module CSS files inline with our CSS standards

Remaining tasks

Review the current CSS — What to look for when reviewing CSS

User interface changes

None

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because coding standards
Issue priority Not critical because coding standards
Unfrozen changes Unfrozen because it only changes markup

CommentFileSizeAuthor
#22 Screen Shot 2015-06-12 at 09.50.00.jpg551.13 KBLewisNyman
#22 Screen Shot 2015-06-12 at 09.49.47.jpg419.72 KBLewisNyman
#21 interdiff-2408463-17-21.txt1.24 KBthomasfava
#21 refactor-entity-reference-css-2408463-21.patch4.01 KBthomasfava
#17 interdiff-2408463-4-17.txt1.4 KBthomasfava
#17 refactor-entity-reference-css-2408463-17.patch3.88 KBthomasfava
#12 interdiff-2408463-4-12.txt4.81 KBMathieuSpil
#12 refactor-entity-reference-css-2408463-12.patch7.82 KBMathieuSpil
#9 interdiff-2408463-4-8.txt1.94 KBManjit.Singh
#8 refactor-entity-reference-css-2408463-8.patch4.78 KBVidushi Mehta
#7 Screen Shot 2015-05-10 at 12.40.53.jpg537.67 KBLewisNyman
#4 refactor-entity-reference-css-2408463-4.patch3.91 KBManjit.Singh
#4 place-blocks-expand.png69.95 KBManjit.Singh
#4 place-blocks.png73.26 KBManjit.Singh
#3 refactor-entity-reference-css-2408463-3.patch3.84 KBMathieuSpil
#1 entity-meta.css_.txt1.45 KBLewisNyman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
FileSize
entity-meta.css_.txt1.45 KB
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Issue summary: View changes
MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil as a volunteer commented
Status: Active » Needs review
FileSize
refactor-entity-reference-css-2408463-3.patch3.84 KB

Kicking off this issue:

-.entity-meta details[open] + [open] {
+.entity-meta details[open] + details[open] {

Fixes the only CSSlint-error. (unqualified-attributes)

Changed the .entity-meta-header-selector by the.entity-meta__header-selector. => should we keep the entity-meta start of the selector?

Changed the .entity-meta-header .published-selector by the.entity-meta__header__title-selector. => readability

With the same logic, I also changed .entity-meta-header .changed with .entity-meta__header__last-saved again for readability.

Should we refactor this selector: .entity-meta details into something like .entity-meta__panel? (for me the details-selector is the equivalent of the div-selector and therefor should we avoid using it?)

Is there a reason why on /admin/structure/block we don't use the .entity-meta-selector as a parent of the .entity-meta-header-selector?

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
place-blocks.png73.26 KB
place-blocks-expand.png69.95 KB
refactor-entity-reference-css-2408463-4.patch3.91 KB

@MathieuSpil One thing i have noticed here is when details are not opened then there is no border for first-child, Please find screenshots for more clarity.

MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil as a volunteer commented
FileSize
interdiff-2408463-3-4.txt508 bytes

Added interdiff for readability.

MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil as a volunteer commented

Can someone give us more context to where this entity-meta-wrapper can be found:
Semantics are a tiny bit different from
node/1/edit and admin/structure/block. Are there any more places where this is being used? The solution in #4 fixes a issue on the blocks page, but it is a mere coincidence that it doesn't break the node-edit page. How should this be approached?

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work
FileSize
Screen Shot 2015-05-10 at 12.40.53.jpg537.67 KB

Nice, I ran this through CSSlint and the error is fixed. Also here is screenshot showing that there are no regressions.

Semantics are a tiny bit different from
node/1/edit and admin/structure/block. Are there any more places where this is being used? The solution in #4 fixes a issue on the blocks page, but it is a mere coincidence that it doesn't break the node-edit page.

I think those are the only two places these are being used. See #2375663: Update the design for the right sidebar on content creation page

  1. +++ b/core/themes/seven/css/components/entity-meta.css
@@ -3,48 +3,46 @@
 .entity-meta details {
...
 .entity-meta details {

    Can we rename this selector so we use a class like entity-meta__content instead of the details element? 

  2. +++ b/core/themes/seven/seven.theme
@@ -173,19 +173,22 @@ function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {
+        'class' => 'entity-meta__header__title',

    I think this can be entity-meta__title as well 

  3. +++ b/core/themes/seven/seven.theme
@@ -173,19 +173,22 @@ function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {
+      '#wrapper_attributes' => array('class' => array('entity-meta__header__last-saved', 'container-inline')),

    I think we can simplify this class a little to just entity-meta__last-saved because we don't need to reflect the html structure in the CSS classes

Vidushi Mehta’s picture

Vidushi Mehta CreditAttribution: Vidushi Mehta commented
Status: Needs work » Needs review
FileSize
refactor-entity-reference-css-2408463-8.patch4.78 KB

@LewisNyman i have changed the classes but bit confused about entity-meta details[open] + details[open] .Should we also rename this to the given class?

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
interdiff-2408463-4-8.txt1.94 KB

uploading interdiff just for clarification what @vidushi did in #8.

Status: Needs review » Needs work

The last submitted patch, 8: refactor-entity-reference-css-2408463-8.patch, failed testing.

MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil as a volunteer commented
Assigned: Unassigned » MathieuSpil

Hey Vidushi,

I see that in your patch(#8), you tried to fix the review of Lewis in (#7), which is great!

But if you change a css-selector, you should doublecheck that de html-output will also contain this class, so the new css will apply. (and vice versa)

MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil as a volunteer commented
Assigned: MathieuSpil » Unassigned
Status: Needs work » Needs review
FileSize
refactor-entity-reference-css-2408463-12.patch7.82 KB
interdiff-2408463-4-12.txt4.81 KB

@Lewis: I am agreeing on the fact that the .entity-meta details-selector should become .entity-meta__details for css-readability purposes and smacss, but in this context this will become unmaintainable. Some modules will hook into this and add multiple options. and the logic of defining the classes for these details elements is module-specific. So the question becomes: 'Do module-builders have to know that they should add the .entity-meta__details to their details element?' If not, I wouldn't change it. Also the following css shouldn't no longer be very logical:

.entity-meta details[open] + details[open] {
  background-image: none;
  border-top-width: 1px;
  padding-top: 0;
}

Because the open-attribute is only appended on detail-elements.

In short: The patch (based upon #4) for clarity's sake, shows what that change would imply for modules (But we shouldn't be working further upon this. ). Suggestion 2&3 from #7 should still be implemented after this.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented

@Mathieu That makes sense, but I think that over time this component will become more generic and more reuseable, and module developers will only need to pass through the template file. They wouldn't need to add the class manually. With that in mind, which approach do you think is more maintainable? I would be nice to keep all the selectors as week as possible.

MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil as a volunteer commented

I would say that we can't use a good generic classname untill we provide a template file. So basically we can only use good css-classes if those do not become a problem to the module-developer.

So to summarize the open TODO's for this ticket since the patch in #4
1) Choose between keeping .entity-meta details(#4) and refactoring existing code to .entity-meta__details(#12). (For the pro's and con's you can read the earlier comments)
2) Replace the entity-meta__header__title-selector with the entity-meta__title-selector.
3) Replace the entity-meta__header__last-saved-selector with the entity-meta__last-saved-selector.

MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil as a volunteer commented
Status: Needs review » Needs work
thomasfava’s picture

thomasfava
Primary language Dutch
Location Belgium
CreditAttribution: thomasfava commented
Assigned: Unassigned » thomasfava
thomasfava’s picture

thomasfava
Primary language Dutch
Location Belgium
CreditAttribution: thomasfava commented
FileSize
refactor-entity-reference-css-2408463-17.patch3.88 KB
interdiff-2408463-4-17.txt1.4 KB

I did points 2 & 3 from #14 (replaced the entity-meta__header__title-selector with the entity-meta__title-selector and replaced the entity-meta__header__last-saved-selector with the entity-meta__last-saved-selector).

Point 1 is still open.

thomasfava’s picture

thomasfava
Primary language Dutch
Location Belgium
CreditAttribution: thomasfava commented
Assigned: thomasfava » Unassigned
Status: Needs work » Needs review
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work

I approve of these changes! This looks good. One small thing

+++ b/core/themes/seven/css/components/entity-meta.css
@@ -3,48 +3,46 @@
+.entity-meta__title {
...
   margin: .25em 0;
...
+.entity-meta__header .form-item {
+  margin: .25em 0;

We always use leading 0s in our decimal numbers

thomasfava’s picture

thomasfava
Primary language Dutch
Location Belgium
CreditAttribution: thomasfava commented
Assigned: Unassigned » thomasfava
thomasfava’s picture

thomasfava
Primary language Dutch
Location Belgium
CreditAttribution: thomasfava commented
Assigned: thomasfava » Unassigned
Status: Needs work » Needs review
FileSize
refactor-entity-reference-css-2408463-21.patch4.01 KB
interdiff-2408463-17-21.txt1.24 KB

Added leading 0s in decimal numbers.

Keep in mind point 1) from comment #14 is still open.
(Choose between keeping .entity-meta details #4 and refactoring existing code to .entity-meta__details #12.)

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Component: CSS » Seven theme
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
Screen Shot 2015-06-12 at 09.49.47.jpg419.72 KB
Screen Shot 2015-06-12 at 09.50.00.jpg551.13 KB

Ok thanks, on reflection I think that we've done enough in this issue. There are other issues like #2375663: Update the design for the right sidebar on content creation page that can make this component to become more reuseable and make more sense as a component.

I ran this through CSSlint and it returned no errors.

Screenshots:

Note that there are no screenshots from Stark/Classy/Bartik because there is no entity meta styling anywhere except Seven.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Manually tested and I could not spot any regression. Committed 53c197a and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 53c197a on 8.0.x 
    Issue #2408463 by thomasfava, MathieuSpil, Manjit.Singh, Vidushi Mehta,...

Status: Fixed » Closed (fixed)

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

Vidushi Mehta’s picture

Vidushi Mehta CreditAttribution: Vidushi Mehta as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented