ampproject / amp.dev

The AMP Project Website.
https://amp.dev
Other
583 stars 694 forks source link

Spacing issues in examples #4251

Open morsssss opened 4 years ago

morsssss commented 4 years ago

Working on the <amp-script> examples page, I've hit a couple of snags:

1. Pretty-printing

I find it confusing that HTML attributes for <script> tags get automatically pretty-printed so that each is on a separate line - as that tends to mush into the JavaScript that people will need to focus on. @sebastianbenz showed me that I could defeat this behavior by adding <script> to BEAUTIFY_OPTIONS.unformatted[] in DocumentParser.js':

const BEAUTIFY_OPTIONS = {
  indent_size: 2,
  "wrap_attributes": "force",
  unformatted: ['noscript', 'style', 'head', 'script'],
  'indent-char': ' ',
  'no-preserve-newlines': '',
  'extra_liners': []
};

However, this is having no effect.

2. Indentation

The pretty-printing normally works beautifully for JavaScript in examples. However, I'm having spacing problems in cases where I need to wrap the whole example in a <div> - since AFAIK a single example can't have multiple parent tags, but needs to have a single parent tag, which then gets magically removed.

So, this code:

    <div>
      <amp-script layout="fixed-height" height="36" script="time-script" class="sample">
        <div>The time is: <span id="time"></span></div>
      </amp-script>

      <script id="time-script" type="text/plain" target="amp-script">
        const fetchCurrentTime = async () => {
          const response = await fetch('<% hosts.platform %>/documentation/examples/api/time');
          const data = await response.json();
          const div = document.getElementById('time');
          div.textContent = data.time;
        }

        fetchCurrentTime();
      </script>
    </div>

gets formatted like this:

image

morsssss commented 4 years ago

@matthiasrohmer , I wonder if this is something else we can look at after Pixi?

patrickkettner commented 4 years ago

@morsssss can you explain 1 more? All of the <script>s are one lined on the linked page. Can you share a screenshot of an affected example

morsssss commented 4 years ago

Yeah, you're right. It must have started working at some point, because even in the screenshot I included here, it works 🙄

Sorries!

It would be great to fix #2, though. Until we do, people will think that we don't know how to format JavaScript 😬

patrickkettner commented 4 years ago

that actually appears to be by design.

If you have anything other than the string <div>, it won't be removed

morsssss commented 4 years ago

That's not what I mean, though! I mean, that the following issues occur:

  1. Vertical spaces are sometimes removed from the JavaScript
  2. The code gets indented 4 spaces instead of 2, which isn't a big deal, except that then the closing </script> tag is indented incorrectly.
morsssss commented 4 years ago

And, wait... I found a couple of examples where #1 above is still true: that the <amp-script> attributes each get stuck on their own line. See here and here.

patrickkettner commented 4 years ago

ah! you want <amp-script> to be the same line? In that case, you'd want to add amp-script to the same array as script

  unformatted: ['noscript', 'style', 'head', 'script', 'amp-script'],
patrickkettner commented 4 years ago

Rereading, I am not sure we are on the same page. I'll try to summarize, can you correct what is wrong, @morsssss?

a. you want the attributes for <script> and <amp-script> elements to be on one line. b. Vertical spaces are sometimes removed from <amp-script> samples c. <amp-script> demos get indented incorrectly in some instances.

Is there anything else?

morsssss commented 4 years ago

I'm sorry. I'm just an idiot. Or trying to do too many things at once.

Since I keep mistaking the <amp-script> attributes on different lines for an error, when in fact I deliberately chose not to include that tag in the unformatted array: what do you think? Do you prefer them on separate lines? Or on one line?

"b" and "c" up there is what I meant, yes.

patrickkettner commented 4 years ago

Do you prefer them on separate lines? Or on one line?

don't have a strong feeling either direction, but in general new line if it is important, same line if it isn't

do you have an example of b and/or c?

morsssss commented 4 years ago

For b/c, sure! See this sample.