1545598 - Copy All Changes: Make output more compact by removing extra row breaks and path

Status: NEW

(DevTools :: Inspector: Changes, enhancement, P2)

Description

[Victoria Wang [:victoria]]

Used Copy All Changes for something and got this output:

/* WelcomeBox.css | resource://devtools/client/debugger/src/components/WelcomeBox.css */

.welcomebox {
  /* bottom: var(--editor-footer-height); */
  bottom: 0;
}

I realized I wanted to condense the output like this:

/* WelcomeBox.css */
.welcomebox {
  /* bottom: var(--editor-footer-height); */
  bottom: 0;
}

1. There seems to be an extra row break before the file name
2. Row break between file name and CSS seems unnecessary
3. I feel the full path of the source won't be needed in most cases, and removing it makes the output much easier to take in

Comment 1

[Razvan Caliman [:rcaliman]]

There seems to be an extra row break before the file name

Indeed. I thought I'd fixed that, but I guess I forgot.

Row break between file name and CSS seems unnecessary

Yes, that can be removed.

These formatting changes need to be done in https://searchfox.org/mozilla-central/rev/d302c3058330a57f238be4062fddea629311ce66/devtools/client/inspector/changes/selectors/changes.js#152-237 where the newlines \n for rules are moved from before the selector to after the closing bracket, and removed altogether form the source path. The test also needs updating to match the new format.

I feel the full path of the source won't be needed in most cases, and removing it makes the output much easier to take in

I see how it wouldn't be needed in most cases. But when it is needed, for example when tweaking in two styles.css stylesheets which belong to different components, it will be needlessly difficult for someone to pinpoint the right one.

As an anecdote, myself and others have started sharing this output with changes on code reviews for contributors. The full path of the stylesheet is helpful for them so they know where to find it in the project file tree.

I'd make the case we should keep the full stylesheet path in the output. The full path is the page URL for inline stylesheets or inline styles. We couldn't remove it in those case because that would remove important context.

Comment 2

[Victoria Wang [:victoria]]

I'd make the case we should keep the full stylesheet path in the output. The full path is the page URL for inline stylesheets or inline styles. We couldn't remove it in those case because that would remove important context.

I can see what you mean about the use cases that depend on this. And since this type of export is usually for communication rather than direct-to-code, I guess it's not a big deal to have a line of extra contextual info. Most people are probably less obsessive than me about trimming down all pasted text to the minimum :D

Two more things I just noticed in Nightly:

• Copy All Changes uses two tab characters for the indents, while Copy Rule seems to use 4 spaces. (Did this change recently?)

• When there are multiple rules, we could remove the extra row breaks between rules.

Comment 3

[Razvan Caliman [:rcaliman]]

I can see what you mean about the use cases that depend on this. And since this type of export is usually for communication rather than direct-to-code, I guess it's not a big deal to have a line of extra contextual info. Most people are probably less obsessive than me about trimming down all pasted text to the minimum :D

Ok. Keeping the full stylesheet path.

Two more things I just noticed in Nightly:

• Copy All Changes uses two tab characters for the indents, while Copy Rule seems to use 4 spaces. (Did this change recently?)

Copy All Changes uses the default or preferred indentation set in DevTools Settings > Editor Preferences.

Copy Rule copies the text from the stylesheet as authored. It does not reformat it. Whatever indentation there was, is preserved. The use case for Copy Rule is to provide a chunk of code that can fully replace the rule in the original stylesheet, therefore we want to to preserve the user's formatting style and any comments.

• When there are multiple rules, we could remove the extra row breaks between rules.

I'd favor keeping the newline between rules for readability. Particularly with longer selectors, it can start to look like a wall of text and missing the newline reduces the ability to quickly scan the list of changes.

Comment 4

[Victoria Wang [:victoria]]

Copy All Changes uses the default or preferred indentation set in DevTools Settings > Editor Preferences.

Ah, I see. I'd expect this feature to used the authored indentation, based on the fact that it isn't visually formatted in Rules according to the pref, so I don't expect that value to be involved.

Also, I know Copy All Changes isn't for replacing multiple blocks of CSS, but I'd still expect users to regularly paste some of the lines back into their code editors after communicating about it.

I'd favor keeping the newline between rules for readability. Particularly with longer selectors, it can start to look like a wall of text and missing the newline reduces the ability to quickly scan the list of changes.

I see what you mean about long selectors. I feel that since CSS is almost always viewed with just the } row break, that feels most natural to me, and the addition of empty row breaks looks like it needs to be trimmed.

Assuming the main use case here is pasting the output into a wide text display like in an issue tracker, email, or Slack, I'd expect that there's usually enough room to show long selectors on a single line or two, which then help to separate the blocks.

Just to help visualize, I made a quick example with long selectors. With blank row breaks:

/* Inline #3 | https://www.nytimes.com/subscription.html?campaignId=67HX6&&gclid=EAIaIQobChMI9_7Om4jg3wIVM_7jBx0bdwljEAAYASAAEgIoZfD_BwE */
@media (min-width: 1440px) {
  .header__title .tagline, .header__title .sub-tagline, .header__title .subscription-price, .header__title .urgency-messaging {
    /* font-size: 28px; */
    color: red;
    font-weight: bold;
  }
}

.header__title .tagline {
  /* margin-bottom: 10px; */
}

.header__title .tagline, .header__title .urgency-messaging {
  /* font-weight: 500; */
  font-size: 2em;
  font-stretch: unset;
}

.header__title .tagline, .header__title .sub-tagline, .header__title .subscription-price, .header__title .urgency-messaging {
  /* color: #333; */
  /* font-family: "nyt-franklin", NYTFranklin, Franklin, Arial, sans-serif; */
  /* font-size: 16px; */
  /* font-weight: 700; */
  /* line-height: 1.09090909; */
  /* text-align: left; */
}

@media (min-width: 375px) {
  .header__title .tagline, .header__title .sub-tagline, .header__title .subscription-price, .header__title .urgency-messaging {
    float: left;
    border: 1px solid blue;
    font-style: italic;
  }
}

@media (min-width: 768px) {
  .header__title .tagline, .header__title .sub-tagline, .header__title .subscription-price, .header__title .urgency-messaging {
    color: green;
  }
}

Without extra row breaks:

/* Inline #3 | https://www.nytimes.com/subscription.html?campaignId=67HX6&&gclid=EAIaIQobChMI9_7Om4jg3wIVM_7jBx0bdwljEAAYASAAEgIoZfD_BwE */
@media (min-width: 1440px) {
  .header__title .tagline, .header__title .sub-tagline, .header__title .subscription-price, .header__title .urgency-messaging {
    /* font-size: 28px; */
    color: red;
    font-weight: bold;
  }
}
.header__title .tagline {
  /* margin-bottom: 10px; */
}
.header__title .tagline, .header__title .urgency-messaging {
  /* font-weight: 500; */
  font-size: 2em;
  font-stretch: unset;
}
.header__title .tagline, .header__title .sub-tagline, .header__title .subscription-price, .header__title .urgency-messaging {
  /* color: #333; */
  /* font-family: "nyt-franklin", NYTFranklin, Franklin, Arial, sans-serif; */
  /* font-size: 16px; */
  /* font-weight: 700; */
  /* line-height: 1.09090909; */
  /* text-align: left; */
}
@media (min-width: 375px) {
  .header__title .tagline, .header__title .sub-tagline, .header__title .subscription-price, .header__title .urgency-messaging {
    float: left;
    border: 1px solid blue;
    font-style: italic;
  }
}
@media (min-width: 768px) {
  .header__title .tagline, .header__title .sub-tagline, .header__title .subscription-price, .header__title .urgency-messaging {
    color: green;
  }
}

Comment 5

[Martin Balfanz [:mbalfanz]]

I see what you mean about long selectors. I feel that since CSS is almost always viewed with just the } row break, that feels most natural to me, and the addition of empty row breaks looks like it needs to be trimmed.

Do we have data on this? It sounds more like personal preference rather than "almost always". I, for example, always keep a newline in my CSS files 😅

Comment 6

[Victoria Wang [:victoria]]

Haha, good point :). I guess I mean I'm really used to no empty lines from projects I've worked on - for vanilla CSS at least. I realized I did use newlines in SASS, if I remember correctly, to help with the more complex code-like appearance.

Feels like a tabs vs spaces kind of problem and I'm fine deferring to the more expanded style. If we do have empty lines though, we should keep the empty line after the path comment, to keep a consistent amount of separation.