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

resolved code formatting issue #371 #385

Merged
merged 24 commits into from
Mar 5, 2024
Merged

Conversation

Adity20
Copy link
Contributor

@Adity20 Adity20 commented Feb 28, 2024

GitHub Issue: #371

Summary: This pull request is being made proactively to fix a particular problem which is code formatting for the issue #371.
Resolves #371

Screenshot 2024-02-28 181455
the UI looks like this now and if any other changes are required kindly let me @benjagm.

@benjagm kindly review my PR

@@ -196,7 +200,7 @@ Here's how you would covert a schema using `$recursiveRef` to use `$dynamicRef`.
}
}
}
// strict-tree schema, guards against misspelled properties
// strict-tree schema, guards against misspelled properties
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there's still an issue with syntax highlighting. Probably is this comment.

Can you change the language specifier to jsonc to see if that fixes the issue?

@Adity20
Copy link
Contributor Author

Adity20 commented Feb 29, 2024

Sure @gregsdennis I'll try that too.

@benjagm benjagm added the Status: In Progress This issue is being worked on, and has someone assigned. label Feb 29, 2024
@Adity20
Copy link
Contributor Author

Adity20 commented Feb 29, 2024

Hi @gregsdennis I have tried some changes and now the UI looks like this
Screenshot 2024-02-29 163342
Screenshot 2024-02-29 163351
kindly review it and let me know if some more changes are required

@Adity20 Adity20 requested a review from gregsdennis March 1, 2024 06:14
```json
// tree schema, extensible
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there's an incomplete merge in here.

<td>

```json
{
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the indention is being rendered. Can you pull this back to the start of the line, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure @gregsdennis i'll make the changes asap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you check now @gregsdennis

Copy link
Member

Choose a reason for hiding this comment

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

Did you push the changes? I don't see a difference in the code. Also, could you provide a screenshot? I can't run the site locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but at my end its showing the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here are the screenshots
Screenshot 2024-03-02 114910
Screenshot 2024-03-02 114924
Screenshot 2024-03-02 114939

and the output of these changes
Screenshot 2024-02-29 163342
Screenshot 2024-02-29 163351

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the indention changes I asked for. This is what's confusing.

Please move the JSON all the way to the left, ignoring the HTML indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now fine @gregsdennis ?
Screenshot 2024-03-02 140956

Copy link
Member

Choose a reason for hiding this comment

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

There's still some indention. Please pull all the way to the left.

image

@Adity20
Copy link
Contributor Author

Adity20 commented Mar 2, 2024

Hi @gregsdennis I changed the format to jsonc and now the output looks like this kindly check
Screenshot 2024-03-02 221450

@gregsdennis
Copy link
Member

gregsdennis commented Mar 2, 2024

What's going on here?

image

Also, the indentation seems to have been messed up on the 2020-12 and completely removed on the 2019-09. It's almost as if it's rendering spaces with variable width.

image

This is very confusing; the code looks fine to me. Thanks for hanging in on this one.

@Adity20
Copy link
Contributor Author

Adity20 commented Mar 3, 2024

Now fine @gregsdennis ?
Screenshot 2024-03-03 083545

@gregsdennis
Copy link
Member

The code looks good, but it still doesn't render right. The code has the correct indentation, but the render shows none.

@Adity20
Copy link
Contributor Author

Adity20 commented Mar 3, 2024

So what should I do now?

@benjagm
Copy link
Collaborator

benjagm commented Mar 3, 2024

Maybe this is related with the problems here:

There are 2 config attributes in the code editor I needed to change to avoid extra width of the containing page for some scenarios for small devices, but the solution was suboptimal. I think this is related. Below the commit I did:
cd30786

related (maybe): react-syntax-highlighter/react-syntax-highlighter#376

@Adity20
Copy link
Contributor Author

Adity20 commented Mar 3, 2024

Maybe this is related with the problems here:

There are 2 config attributes in the code editor I needed to change to avoid extra width of the containing page for some scenarios for small devices, but the solution was suboptimal. I think this is related. Below the commit I did: cd30786

related (maybe): react-syntax-highlighter/react-syntax-highlighter#376

Is this related to issue #393?

@benjagm
Copy link
Collaborator

benjagm commented Mar 3, 2024

The problem is with indentation right? Those 2 parameters affects exactly that. First play with them, later well need to check how to make sure the component's behavior is correct in other pages, specifically in small devices.

@Adity20
Copy link
Contributor Author

Adity20 commented Mar 3, 2024

Okay @benjagm I'll try that too.

@benjagm
Copy link
Collaborator

benjagm commented Mar 4, 2024

@Adity20 did you tested the component indentation disabling the attributes wrapLines and wrapLongLines? I think that can fix it.

@Adity20
Copy link
Contributor Author

Adity20 commented Mar 4, 2024

Yes I am testing out all the changes as you mentioned @benjagm will be done by tomorrow.

@Adity20
Copy link
Contributor Author

Adity20 commented Mar 5, 2024

I tried disabling these two attributes and tried some other changes too but that's creating an overflow in the page as the code is taking more space now to be in the same line.
I don't think this will work we'll have to apply more custom styles maybe @benjagm

@gregsdennis
Copy link
Member

Can we have each section scroll horizontally? Would it be weird to have that here but not in other places?

Alternatively, what if we made it look like a git diff, where you do a red - line followed by a green +line? Is that possible?

@benjagm
Copy link
Collaborator

benjagm commented Mar 5, 2024

Can we have each section scroll horizontally? Would it be weird to have that here but not in other places?

Alternatively, what if we made it look like a git diff, where you do a red - line followed by a green +line? Is that possible?

I'd suggest fix everything but the indentation issue here and create an different issue to configure the code component to have its own horizontal scroll bar se fix this across the entire site.

@gregsdennis
Copy link
Member

I'm happy with that

@Adity20
Copy link
Contributor Author

Adity20 commented Mar 5, 2024

So I should try horizontal scrollbar once or keep trying some other changes?

@benjagm
Copy link
Collaborator

benjagm commented Mar 5, 2024

@Adity20 I think si better to do it in a separate issue. Let suggest this:

  • Let's approve this leaving the indentation unfixed.
  • I'll open the issue and assign it to you if this is ok for you.

Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

Great job @Adity20 !!!

@Adity20
Copy link
Contributor Author

Adity20 commented Mar 5, 2024

Yeah that works for me @benjagm. I can start working on the that one then.
And thanks for constant support always.

@benjagm
Copy link
Collaborator

benjagm commented Mar 5, 2024

@Adity20 please add a comment in #456 to get assigned.

Copy link
Member

@gregsdennis gregsdennis left a comment

Choose a reason for hiding this comment

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

Everything except the indentation seems to be resolved. I'm happy address that elsewhere.

@benjagm benjagm merged commit 316d2a6 into json-schema-org:main Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In Progress This issue is being worked on, and has someone assigned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code formatting issue in the release notes of Draft 2020-12
3 participants