Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code snippet details renderer type #6901

Closed
mattzeunert opened this issue Dec 31, 2018 · 9 comments
Closed

Code snippet details renderer type #6901

mattzeunert opened this issue Dec 31, 2018 · 9 comments

Comments

@mattzeunert
Copy link
Contributor

mattzeunert commented Dec 31, 2018

We're building this primarily for JSON-LD validation results, but are hoping to re-use it elsewhere eventually.

The type looks like this:

interface DetailsRendererCodeSnippetItem {
      type: "code-snippet",
      code: string,
      title: string,
      highlights: {
          line: number | null,
          message: string
      }[]
} 

Two rendered items:

screenshot 2018-12-31 at 21 38 53


Putting the code-snippet items into a single-column table looks bad. I'm planning to create another item type code-snippet-list to render them. Does that sound reasonable?

I guess it could also be called lh-list since it's really just a list with padding between items.

Not sure if we'd ever want it to be part of a table – here's how that would look like right now.

What do you think about how to structure the item types here?

@exterkamp
Copy link
Member

I like this look, I definitely agree that a 1 column table would look odd. Definitely like the look of a full width code-snippet list.

I feel like this could be an lh-list or maybe lh-full-width-list?

Does each snippet contain the full JSON-LD file for expansion? Should it be a fixed # of lines, I don't see why we would need to include an expansion on each snippet as long as we would preserve the overall global line numberings. Just like the +/- 2 lines from the error.

@hwikyounglee thoughts on the look and feel? Stylistically I might want the gray outlines to go away to match the full width tables' style.

@mattzeunert
Copy link
Contributor Author

Does each snippet contain the full JSON-LD file for expansion?

Yeah. If the JSON can be parsed we show a prettified version of the snippet.

There are two goals here:

  • show the user why there is a failure
  • allow the user to identify the failing JSON-LD tag

Usually the snippet will be less than 20 lines long, so it won't affect the result size too much. Beyond that we can truncate it.


Just in case someone finds it helpful, here are some more examples of what the item type looks like.

@mattzeunert
Copy link
Contributor Author

@rviscomi brought up linking the JSON-LD item to the node in the DevTools elements panel.

The linking works by looking for .lh-node elements, so we could render one of those and slightly restyle it. Or we can tweak the DevTools code to pick up data-path attributes.

Do we want the UI to look something like this?

screenshot 2019-01-06 at 09 42 56

@connorjclark
Copy link
Collaborator

Or we can tweak the DevTools code to pick up data-path attributes.

The DT code already uses data-path to link to a node.

image

Or are you suggesting the DT code just looks for any element (not just .lh-node) for data-path?

@mattzeunert
Copy link
Contributor Author

Or are you suggesting the DT code just looks for any element (not just .lh-node) for data-path?

Yeah. Not sure what the final UI will be for the JSON-LD snippets, but I feel it'd be better to avoid lh-node rather than overriding its styles.

@mattzeunert
Copy link
Contributor Author

Current status

Got some design feedback from @hwikyounglee https://jsonld-display.glitch.me/#seo

screenshot 2019-01-13 at 11 46 27

Limiting snippet size

I started truncating the snippets, currently storing 20 lines around each highlight, with a max line length of 200.

New details renderer data:

export interface DetailsRendererCodeSnippet {
  type: "code-snippet",
  lines: DetailsRendererCodeSnippetLine[],
  title: string,
  highlights: DetailsRendererCodeSnippetHighlight[]
  lineCount: number
}

export interface DetailsRendererCodeSnippetHighlight {
  /** Line number, starting from 1 */
  lineNumber: number | null,
  message: string
}

export interface DetailsRendererCodeSnippetLine {
  content: string
  /** Line number, starting from 1 */
  number: number
  truncated?: boolean
}

Linking to elements panel

Going to put this next to the title, but only show it in DevTools.

Will just use .lh-node instead of having DevTools pick up [data-path].

@hwikyounglee
Copy link

Thanks Matt!

Pasting follow-up comments, and feel free to take the ones only that make sense.

  1. For the following case, i.e. error message before code
    image
    Does it make sense to swap the order so an error message always come after code?
  2. Would we wrap the error message to see it without scrolling?
    Mock for 1 and 2:
    image
  3. For JSON in a single line
    image
    Does it make sense to do some kind of pretty-print by wrapping at commas?

@mattzeunert
Copy link
Contributor Author

mattzeunert commented Jan 15, 2019

Thanks Hwi!

Does it make sense to swap the order so an error message always come after code?

If the message appears below a specific line it feels like it belongs to that particular line, even though all lines are highlighted. I'd prefer keeping error messages that don't belong to a particular line at the top.

Would we wrap the error message to see it without scrolling?

I've partially implemented this. If the code itself is scrollable the error text still scrolls, will see if I find a clean way to prevent that in CSS.

screenshot 2019-01-15 at 14 15 16

For JSON in a single line – Does it make sense to do some kind of pretty-print by wrapping at commas?

That would be nice to have 95% of the time, but it's tricky to get right. If we just put a line break after each comma we'd also put line breaks in the middle of strings that contain commas, and the user might think those line breaks are why the JSON is invalid. Given that the JSON is invalid I don't think we can find a 100% safe way to prettify it.

What would be helpful here is a column index for the error or a better error message. I'll check with @rviscomi, but got the feeling that'll be lower priority.

Btw, it'll only be single line if the JSON is invalid and it's single line in the page HTML.

@hwikyounglee
Copy link

Got it! Thanks for ramping me up on all these points. Thanks Matt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants