Open tlawrie opened 3 months ago
Hey @tlawrie, I really appreciate you looking into this and exploring what it'd take to move to rehype-pretty-code
. Let's continue the conversation in this PR, just so it's consolidated. I appreciate all of the info in #237, it's very helpful in understanding the tradeoffs.
I do like that this removes a lot of our own custom code, it definitely feels more maintainable to drop ~100 lines of code. I do have a few concerns though about how this affects the styles of our docs.
In all of the examples I'm including a side-by-side of remix.run (left) and this PR (right)
The two syntax changes would require us to go update all of our existing docs, which would already be a good bit of work and potentially cause some disruptions for inflight PRs. Plus, we would need to do the same thing with React Router. This isn't a dealbreaker if we felt the change made a big enough positive impact on the maintainability of remix.run and reactrouter.com.
However, a bigger issue is that this creates a backwards incompatible change. Our docs are setup in such a way that you can look at the docs of any previous version, which is incredibly helpful when your team is upgrading from a version like v1 to v2 (and v2 to react-router v7). Each of these versions uses the docs tagged with the appropriate version in GitHub, so we're not going to go back and change those files to match the new syntax. Here's an example from v2.1.0 as an example of how this is going to disrupt all previous documentation
Changing the copy button's placement and making it omnipresent doesn't seem like it's a compromise worth making to me. One issue I see immediately is that it will overlap with the content if the text is long enough or the screen size is too small.
I also have a small concern that the @rehype-pretty/transformers
package is listed as still being in development
Do you think these issues can be accounted for? Or would the amount of work to circumnavigate the issues result in the same complexity as the current shiki implementation?
Not a problem at all. Happy to work through this as I understand it does have a broader impact on two in flight projects.
Its not only the code line removal, but the maintenance, vulnerability patching, plus the upgrade path to the newer shiki etc.
Its understandable there is concern around work in development, i do however think the good part here is that it seems to be an active project. Overall I would put this in the positive column.
The styling differences you have mentioned are to do with the title and line highlighting meta strings being different. Once the correct meta strings are provided, the styling is pretty much exact.
I dove into the transformer code, and it was actually working via hover of the code block. Needed to fix a few styling issues. It now works very closely to how it previously did yet with no custom code other than css.
In understanding this a bit further, I do think this is probably the biggest hurdle. I am not entirely sure yet, of how to over come this.
Options
lines=
but did come across #
as the prefix. {x,z}
does seem to be the standard though.We can actually handle it without needing any adjustments. We can use the filterMetaString
option to create a meta parser to take care of the change for us.
filterMetaString: (str) => str.replace(/lines=\[([^]*)\]/g, '{$1}').replace(/filename=([^ ]*)/g, 'title="$1"'),
@brookslybrand give this a shot now.
I am wanting to propose though that we change the copy button to be like the GitHub code block copy button. Where it has a border and a background for when it does appear over text.
But your call on that. It should only be css styling
@brookslybrand the button would look like this if we adopted how github / shadcn styling (and im sure many others). Makes it a bit clearer when overlapping text. I haven't committed the change.
We can actually handle it without needing any adjustments. We can use the
filterMetaString
option to create a meta parser to take care of the change for us.filterMetaString: (str) => str.replace(/lines=\[([^]*)\]/g, '{$1}').replace(/filename=([^ ]*)/g, 'title="$1"'),
Perfect! This is exactly the kind of solution I was hoping for. This addresses this issue for me
@brookslybrand the button would look like this if we adopted how github / shadcn styling (and im sure many others). Makes it a bit clearer when overlapping text. I haven't committed the change.
Nice, yeah I like that. I'd have to check that the team is good with that, but go ahead and commit it. I actually kind of like the copy button being in the body of the code. I remember that I used to think I was copying the filename when I first used these docs, based on the placement.
I did notice another bug though. Right now you can tab to the copy button (there probably should be an outline when you tab to it, but that's another issue). On this branch tabbing focuses the entire codeblock, but there's nothing you can actually do with it 😕. You should be able to copy the code via the copy button without using your mouse
https://github.com/remix-run/remix-website/assets/12396812/697ec4f8-f079-42c4-8876-ed44521dea83
Hi @brookslybrand good catch
The button can now be tabbed to. Of note, the code block tabs first (which is different to current - not sure if issue)
And styling to match always present with a hover
Hey @tlawrie, not sure what's going on, but I pulled the latest and this is what it looks like for me. Are you having the same experience?
https://github.com/remix-run/remix-website/assets/12396812/0f2f1687-0508-4c1e-9095-7689906a7620
That's my mistake. I picked it up on the boomerang-io/remix-docs-template fork after I merged in for this PR.
It was in the md.server.ts
i had incorrectly referenced var rather than rgb in the stroke of the svg.
Hey @brookslybrand how has it gone? any further thoughts?
Hey @tlawrie, so sorry for the slowdown on my side, I was at an offsite all last week. Thanks for fixing the accessibility! A few more notes:
Sorry there's been so much back and forth on this. I definitely appreciate the transition to the theming, it definitely seems like it'll improve the maintainability. Changes like these always take longer when style changes are being made at the same time, so the more we can keep it a 1-to-1 match with the current style and experience the easier it will be to approve
https://github.com/remix-run/remix-website/assets/12396812/aa1de2cf-becc-43bd-9c4f-6c88dada0ee2
Hey @brookslybrand apologies for the delay on my end this time!
Button Styling We set the button to always hover, due to the fact that the hover is only for the button and not for the entire code block. I.e. you would have to know the button exists to cause the hover.
That, and a number of comparable docs sites have it to always hover.
If you are wanting the styling format you had before for the button, we may have to keep the button code and just adjust it to to work with the new code block.
Dark Mode I didn't actually test dark mode, to be honest, forgot that it existed. Ill take a look at this one.
All good @tlawrie!
If you change the visibility
on transformerCopyButton
to hover
, it appears when you hover over any part of the codeblock. However, you can no longer tab to it with this turned on, so that's a bit of a deal breaker 😔
https://github.com/user-attachments/assets/e5432944-aa77-4612-adf2-a77d9d25de50
You are right though that many docs have an always visible copy button, so I agree, I think that'd be fine so long. And yes, I'd prefer to keep the old button styles. I'm all for switching to this framework, but I'd really prefer for the appearance to stay the same. Essentially, as much as possible, it should just be a code refactor.
No worries about not testing on dark mode, glad to bring it up!
Issue: #237
Description
To remove the added burden of maintaining custom code and attempting to handle major upgrades with changes to Shiki package dependency (currently stuck on a 0.x major version), this PR involves removing the code and replacing it with a rehype module that handles the same thing.
Changes
Differences / Impact
Required changes to the MD meta strings
Other cosmetic differences: