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

feat: facelift to console schema snippets #2804

Merged
merged 2 commits into from
Sep 24, 2024
Merged

feat: facelift to console schema snippets #2804

merged 2 commits into from
Sep 24, 2024

Conversation

deniseli
Copy link
Contributor

Fixes #2759

  • Snippet text comes from module schema
  • In the backend we have some custom grouping rules to reduce vertical spacing, which would be nice to reproduce in the frontend. They're pretty small and keeping them in sync isn't critical, so the rules can probably just be copy pastad.
  • Drop triangle and add border
  • Syntax highlighting
  • Make the snippets handle page borders - position to the top / left when there is not enough space on the screen

styling and parsing

moar

cleanup

to left and right

resizing

overlay

cleanup

lots of cleanup

leading spaces
@deniseli deniseli marked this pull request as ready for review September 24, 2024 21:17
@deniseli deniseli requested review from a team and safeer and removed request for a team September 24, 2024 21:17
This was referenced Sep 24, 2024
@deniseli deniseli requested a review from wesbillman September 24, 2024 21:31
Copy link
Contributor

@safeer safeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I just skimmed the React stuff though 😅

const line = lines[foundIdx]
let out = line
let subLineIdx = foundIdx + 1
while (subLineIdx < lines.length && lines[subLineIdx].startsWith(' ')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: does it make sense to do startsWith(' (\\w+)') here as below instead of 4 spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question! So the behavior here is slightly different - basically, the (\\w+) extracts the decl type from the line but ignores the start of the line, whereas this line is just checking the nesting depth from the start of the line.


// Scan backwards for comments
subLineIdx = foundIdx - 1
while (subLineIdx >= 0 && lines[subLineIdx].startsWith(commentPrefix)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Comment on lines +21 to +22
<LinkToken token={splitToken[0]} containerRect={containerRect} />
(<UnderlyingType token={splitToken[1]} containerRect={containerRect} />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to check for splitToken.length before these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. It should never happen, but if we add a new caller that doesn't check for it, then yeah the guard would definitely come in handy

export const specialChars = ['{', '}', '=']

export function isFirstLineOfBlock(ll: string[], i: number): boolean {
export function shouldAddLeadingSpace(ll: string[], i: number): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are lls in these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to lines

Comment on lines +91 to +92
export function declFromModuleSchemaString(declName: string, schema: string) {
const lines = schema.split('\n')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of this function and file seems to be scanning the schema string to pull it back apart into decls, but we can get the raw schema as well and not have to deal with regexing on the string if that's easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

posting update from our slack discussion: we'll do that with the new endpoint: #2805

@deniseli deniseli merged commit 294535d into main Sep 24, 2024
91 checks passed
@deniseli deniseli deleted the dli/snippet-v2 branch September 24, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

console snippet improvements
3 participants