Follow-up to #1821548: Add a "diff" of some kind to the CMI UI

In working on #2396473: Add missing RTL rules to System CSS, it became apparent to me that some of the css classes being fixed in the patch could not be found anywhere in core. @YesCT helped me discover @heyrocker commented the same thing in #1821548-49: Add a "diff" of some kind to the CMI UI. Shouldn't these classes be removed from system.diff.css?

  1. td.diff-prevlink
  2. td.diff-nextlink
  3. .diff-inline-metadata
  4. .diff-inline-legend
  5. .diff-inline-legend span
  6. .diff-inline-legend label
  7. diff-deleted
  8. diff-changed
  9. diff-added
  10. diff-section-title
  11. diff-content
  12. .odd
  13. .even

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 CSS
Prioritized changes The main goal of this issue is code clean-up
Disruption Non-disruptive because only unused CSS is being removed
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Alka Kumari’s picture

Assigned: Unassigned » Alka Kumari
Alka Kumari’s picture

Assigned: Alka Kumari » Unassigned
pguillard’s picture

Status: Active » Needs review
FileSize
874 bytes

they seem not to be used anywhere indeed

prajaankit’s picture

LewisNyman’s picture

Title: Unused css classes in core » Unused css classes in system.diff.css
Status: Needs review » Needs work
Issue tags: +frontend, +Novice

I looked for these classes and couldn't find them in core. I think there might be a few more classes that aren't being used in this file:

diff-deleted
diff-changed
diff-added
diff-section-title
diff-content

This can go as well:

table.diff .even, table.diff .odd {
  background-color: inherit;
  border: none;
}
pguillard’s picture

pguillard’s picture

Status: Needs work » Needs review
JMC’s picture

Reviewing this at DrupalCampNorth sprint today with @LewisNyman.
Everything seems correct except for the references to the odd and even classes don't seem to have been removed in the patch. Lewis said this references the work here #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors.
I'll try to add this to the patch...

LewisNyman’s picture

Status: Needs review » Needs work
JMC’s picture

JMC’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Great! Thanks for making these changes. Less CSS!

lauriii’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Can we get screenshots before and after of config being diffed through the UI.

Also I don't think we're using...

table.diff .diffchange {
  color: #f00;
  font-weight: bold;
}
LewisNyman’s picture

Issue tags: -Novice +Needs screenshots
pjbaert’s picture

@alexpott
After a search, I noticed that there is still a reference to .diffchange in the HWLDFWordAccumulator.php file

protected function _flushGroup($new_tag) {
    if ($this->group !== '') {
      if ($this->tag == 'mark') {
        $this->line = SafeMarkup::format('@original_line<span class="diffchange">@group</span>', ['@original_line' => $this->line, '@group' => $this->group]);
      }
      else {
        $this->line = SafeMarkup::format('@original_line@group', ['@original_line' => $this->line, '@group' => $this->group]);
      }
    }
    $this->group = '';
    $this->tag = $new_tag;
  }
pguillard’s picture

FileSize
19.21 KB
19.28 KB

Here are the screenshots :

Before :
Before

After :
After

pguillard’s picture

Status: Needs work » Needs review
FileSize
19.21 KB
19.31 KB
LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

Great, thanks for adding the screenshots. Setting back to RTBC based on the comment in #16.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 346567c and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 346567c on 8.0.x
    Issue #2502113 by pguillard, JMC, prajaankit, LewisNyman, pjbaert:...

Status: Fixed » Closed (fixed)

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