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

style: Don't convert single \n to <br> #3414

Merged
merged 6 commits into from
Aug 14, 2024

Conversation

leonardehrenfried
Copy link

@leonardehrenfried leonardehrenfried commented Aug 28, 2023

As described in #3155 we (OpenTripPlanner) tend to have quite long, multi-paragraph descriptions of our fields.

In Markdown a single \n is ignored and only two \n should lead to a new paragraph being started.

markdown-it makes this fairly easy to achieve, which is why this PR is tiny.

Before

Screenshot from 2023-08-28 12-59-50

After

Screenshot from 2023-08-28 13-00-02

I've also taken the liberty to update the dev instructions.

Many thanks for this wonderful tool!

@changeset-bot
Copy link

changeset-bot bot commented Aug 28, 2023

🦋 Changeset detected

Latest commit: 470290c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
graphiql Patch
@graphiql/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 28, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@leonardehrenfried leonardehrenfried changed the title Don't convert single \n to <br> style: Don't convert single \n to <br> Aug 28, 2023
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.27%. Comparing base (f2a6b39) to head (470290c).
Report is 1 commits behind head on graphiql-v4.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           graphiql-v4    #3414   +/-   ##
============================================
  Coverage        67.27%   67.27%           
============================================
  Files              120      120           
  Lines             7004     7004           
  Branches          2261     2266    +5     
============================================
  Hits              4712     4712           
  Misses            2275     2275           
  Partials            17       17           
Files Coverage Δ
packages/graphiql-react/src/markdown.ts 100.00% <ø> (ø)

@acao
Copy link
Member

acao commented Sep 26, 2023

@leonardehrenfried thank you for this, this is exactly how it should behave, I agree. I recall there was disagreement some years back and again with the redesign about this, I will look up some issues/discussions/? when I have more time. @thomasheyenbrock or @jonathanawesome what do you think?

one last thing, can you add a changeset as per the changset bot comment? then it'll be ready to go!

(we used to use conventional commits with lerna but contributors had issues, switched to changesets but I miss conventional commits often. i'm looking into ways to perhaps combine them, so many ways to go about it!)

@leonardehrenfried
Copy link
Author

@acao I've added a changeset. I hope I'm bumping the right packages.

@acao
Copy link
Member

acao commented Sep 27, 2023

awesome, thanks @leonardehrenfried ! I think we would make this a minor release for @graphiql/react as well

@leonardehrenfried
Copy link
Author

Changeset was updated.

@leonardehrenfried
Copy link
Author

Could I perhaps have another review for this?

@acao
Copy link
Member

acao commented Nov 20, 2023

hey thanks for this! i'll come back to this later today or tomorrow and release it with some other new features for this @graphiql/react minor release

@leonardehrenfried
Copy link
Author

Lovely thanks!

@leonardehrenfried
Copy link
Author

Could I perhaps have another review for this, please?

@leonardehrenfried
Copy link
Author

@acao Isn't this good to merge?

@acao
Copy link
Member

acao commented Apr 18, 2024

sorry I hope to have more time for this in the coming months. my current commitments are for my day job and for a grant to work on the GraphQL LSP server that I'm wrapping up

@leonardehrenfried
Copy link
Author

I totally understand. Thanks for maintaining GraphiQL!

@acao
Copy link
Member

acao commented Apr 19, 2024

i think the only thing still stumping me is whether this constitutes a breaking change for some use cases, and whether there is some kind of markdown it config that solves the issue this intended to solve? maybe it was a workaround for the previous markdown library, was it marked? I need to look at the original history from, say, tag 0.7.y era or earlier to determine what the original purpose was

@dimaMachina
Copy link
Collaborator

dimaMachina commented Aug 14, 2024

I agree with @acao that this should not affect the current stable version of GraphiQL, let's fix change rendering it in GraphiQL v4

@dimaMachina dimaMachina changed the base branch from main to graphiql-v4 August 14, 2024 18:00
@dimaMachina dimaMachina merged commit f8b719f into graphql:graphiql-v4 Aug 14, 2024
12 checks passed
@acao acao mentioned this pull request Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants