graphiql icon indicating copy to clipboard operation
graphiql copied to clipboard

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

Open leonardehrenfried opened this issue 1 year ago • 15 comments

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!

leonardehrenfried avatar Aug 28 '23 11:08 leonardehrenfried

🦋 Changeset detected

Latest commit: 470290cef8a4cbabd70a2429f7c2a2b38b2822d4

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

changeset-bot[bot] avatar Aug 28 '23 11:08 changeset-bot[bot]

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: leonardehrenfried / name: Leonard Ehrenfried (687b28e0298cbc41563b34f4b6f653d45e267e21, a6e9612db76b89ee3ffc9e8cb83ca66306a722a2, ce09e53ed59e4909b5cb0db4ad58e2a5fa7c7d52)
  • :white_check_mark: login: dimaMachina / name: Dimitri POSTOLOV (470290cef8a4cbabd70a2429f7c2a2b38b2822d4, b4c38d05457e58fec738691647dc74f961e2fed8, a81beacfc9e361899d5c2d1b78ee06d620d256e6)

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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% <ø> (ø)

codecov[bot] avatar Sep 26 '23 20:09 codecov[bot]

@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!)

acao avatar Sep 26 '23 21:09 acao

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

leonardehrenfried avatar Sep 27 '23 15:09 leonardehrenfried

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

acao avatar Sep 27 '23 16:09 acao

Changeset was updated.

leonardehrenfried avatar Sep 27 '23 17:09 leonardehrenfried

Could I perhaps have another review for this?

leonardehrenfried avatar Oct 12 '23 20:10 leonardehrenfried

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

acao avatar Nov 20 '23 12:11 acao

Lovely thanks!

leonardehrenfried avatar Nov 20 '23 12:11 leonardehrenfried

Could I perhaps have another review for this, please?

leonardehrenfried avatar Jan 22 '24 11:01 leonardehrenfried

@acao Isn't this good to merge?

leonardehrenfried avatar Apr 10 '24 20:04 leonardehrenfried

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

acao avatar Apr 18 '24 05:04 acao

I totally understand. Thanks for maintaining GraphiQL!

leonardehrenfried avatar Apr 18 '24 12:04 leonardehrenfried

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

acao avatar Apr 19 '24 12:04 acao

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 avatar Aug 14 '24 17:08 dimaMachina