Problem/Motivation

In #3123832: [META] Fix @todo items referencing closed issues we've discovered there's a @todo in core/themes/claro/css/theme/media-library(.pcss).css.
This @todo is linked to an d.o. issue that is already closed: #3064914: Views preview adds margin top to views rows.

This is the @todo:

/* @todo Remove in https://www.drupal.org/project/drupal/issues/3064914 */
.views-live-preview .media-library-view div.views-row + div.views-row {
  margin-top: 0;
}

Steps to reproduce

Proposed resolution

#3064914: Views preview adds margin top to views rows was closed as a duplicate of #2272533: Excessive margin-top in Views live preview. That one is currently Postponed (maintainer needs more info).
In #2272533-22: Excessive margin-top in Views live preview @alexpott states:

I'm not sure this is a good change. I played around with this by changing a view to use a grid and when there were enough items to make it use multiple rows the fact there was no margin made it look very very odd. OTOH we don't have any margin for the columns so meh. I dunno. It really depends what's in your views. I do think that in quite a few cases this will render the live preview harder to look at and so I think we should re-consider removing the margin completely. Tricky.

Gonna set to needs review to elicit a few more thoughts.

We need to decide if the approach of deleting the (P)CSS lines is what we want and if so, proceed with #2272533: Excessive margin-top in Views live preview and update the @todo to point to that issue.

If the approach is outdated/unwanted, we should delete the @todo.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3337462

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Spokje created an issue. See original summary.

spokje’s picture

spokje
Primary language Dutch
commented

Pinged @lauriii and @bnjmnm on Slack (https://drupal.slack.com/archives/C1BMUQ9U6/p1675061378201859) to get some Claro maintainer input.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

I think it's simplest if the todo get removed from Claro. The issue (and the issue it was considered a duplicate of) is subjective and may not happen at all. It's also not something that needs to be categorized as temporary to justify it's existence in the codebase, so I think it's better off without the @todo.

I updated the issue summary for #2272533: Excessive margin-top in Views live preview to include removing the media-library targeted styling. The instructions are now documented in the issue itself, which serves the same purpose as the @todo, but moves it somewhere that it's ok for it to be a possibility vs a @todo which suggests an inevitability.

spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes

Thanks @bnjmnm, added an MR to remove the @todo

Spokje opened merge request !3346

spokje’s picture

spokje
Primary language Dutch
commented
Status: Active » Needs review
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Reviewed & tested by the community

No need for a @todo as all the visiblity it needs to be addressed is in the issue summary of #2272533: Excessive margin-top in Views live preview, and since that issue may never land it's probably best to not to cite it in the codebase with a @todo.

  • catch committed 6201fbbd on 10.1.x 
    Issue #3337462 by Spokje, bnjmnm: Update @todo comment in core/themes/...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 6201fbb and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

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

Spokje closed merge request !3346