-
Notifications
You must be signed in to change notification settings - Fork 137
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
Docusaurus upgrade #910
Docusaurus upgrade #910
Conversation
✅ Deploy Preview for lambent-kulfi-cf51a7 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Ignore the above preview - it requires some changes to netlify. |
Thoughts on content Page: /docs/references
I assume this aesthetic was intentional, but it might be nice if they were still links that you could actually click on. Page: /docs/supported-platforms I like the aesthetic of that code block. I think that having separate code blocks for npm, yarn, and pnpm works just fine. That said, I just wanted to let you know that I've seen alternatives to it that you might like. On the docusaurus page, they have tabs for npm/yarn/pnpm, so that you can select the code that works for your package manager. I believe that uses npm2yarn Page: /docs/api/spec#recommended-user-channel-set
Page: /docs/api/ref/DesktopAgent#addintentlistener Personal preference, but I think using an ordered list might break up this paragraph better:
Sometimes MUST and SHOULD are bolded, and sometimes they aren't. Is there a style guide to recommend whether or not that text should be bolded? -- Use cases: Why are the use cases numbered and numbers are skipped? I feel like you're just calling attention to a lack of use cases 6, 7, 8, 11, 12, and 14... |
Hi @julianna-ciq, This is good analysis, but I think it's worth pointing out that I've only really done the change from Docusaurus v1-> v2, so a lot of the stuff you're commenting on is actually things I haven't changed! Problem is, lots of files have moved locations, so the GitHub diff looks like I've changed everything. For matters around the actual contents of the documents, @kriswest is probably your man. |
I will be most happy to approve any proposed layout improvements that don't change the meaning of the standard. I burnt myself out on editing somewhat last year, but really hope to get back to it post this upgrade. In some cases, the things @julianna-ciq is calling out are v2 features we can now use that we couldn't before or that have changed, possibly the callouts and admonitions for example. Re: use-cases: before my time, but I think they kept proposal numbers and so those that were not accepted or later superseded are missing. We haven't done anything more than retain the use-cases since FDC3 1.0 or 1.1 I think... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, these comments, plus my changes in PR #919 should address all outstanding console errors and broken links that I saw.
return <div class={styles.conformanceShowcase}> | ||
{ | ||
relevant.map(c => { | ||
return ( | ||
<div className={styles.conformanceShowcaseItem}> | ||
<div class={styles.conformanceImage}> | ||
<img src={c.conf.image} alt={c.conf.title} title={c.conf.title} /> | ||
</div> | ||
<div class={styles.conformanceText}> | ||
<a href={c.conf.infoLink}><h3>{c.conf.title}</h3></a><ul> | ||
{ | ||
c.badge.items.map(item => { | ||
return (<li class={styles.conformanceItem}> | ||
<p>{item.text} { (item.link) ? <em><a href={item.link}>More Details</a></em> :"" }</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return <div class={styles.conformanceShowcase}> | |
{ | |
relevant.map(c => { | |
return ( | |
<div className={styles.conformanceShowcaseItem}> | |
<div class={styles.conformanceImage}> | |
<img src={c.conf.image} alt={c.conf.title} title={c.conf.title} /> | |
</div> | |
<div class={styles.conformanceText}> | |
<a href={c.conf.infoLink}><h3>{c.conf.title}</h3></a><ul> | |
{ | |
c.badge.items.map(item => { | |
return (<li class={styles.conformanceItem}> | |
<p>{item.text} { (item.link) ? <em><a href={item.link}>More Details</a></em> :"" }</p> | |
return <div className={styles.conformanceShowcase}> | |
{ | |
relevant.map((c, index) => { | |
return ( | |
<div className={styles.conformanceShowcaseItem} key={`showcase-item-${index}`}> | |
<div className={styles.conformanceImage}> | |
<img src={c.conf.image} alt={c.conf.title} title={c.conf.title} /> | |
</div> | |
<div className={styles.conformanceText}> | |
<a href={c.conf.infoLink}><h3>{c.conf.title}</h3></a><ul> | |
{ | |
c.badge.items.map((item, badgeIndex) => { | |
return (<li className={styles.conformanceItem} key={`badge-item-${badgeIndex}`}> | |
<p>{item.text} { item.link && <em><a href={item.link}>More Details</a></em>}</p> |
There were some console errors on this page around the use of class
instead of className
and around not using key
s where necessary.
I have tested this suggestion, and it fixes the console errors.
items.map(sl => | ||
<div className="blockElement fourByGridBlock"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items.map(sl => | |
<div className="blockElement fourByGridBlock"> | |
items.map((sl, index) => | |
<div className="blockElement fourByGridBlock" key={index}> |
There were console errors around the fact that this DIV didn't have the necessary key
prop. I have tested this suggestion, and it addresses the errors.
<div className="conformance-badge"><img src={c.src} /></div> | ||
<div className="conformance-text"><ul> | ||
{ | ||
c.items.map(ti => <li key={ti.link}>{ti.text} { ti.link ? <a className="conformance-details" href={ti.link}>details</a> : "" }</li>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.items.map(ti => <li key={ti.link}>{ti.text} { ti.link ? <a className="conformance-details" href={ti.link}>details</a> : "" }</li>) | |
c.items.map(ti => <li key={ti.text}>{ti.text} { ti.link ? <a className="conformance-details" href={ti.link}>details</a> : "" }</li>) |
There were errors around this LI elements key
prop not being unique. When the key was based on a link (URL), it assumed all of the conformance details would point to different URL's; however several links on this page all point to the same press release.
Since all of the text on this page is different, I've changed the key to be based on the text. In the event that multiple details links will have the same link text, the page will throw errors again.
I have tested the suggested change.
@@ -0,0 +1,160 @@ | |||
import Layout from "@theme/Layout"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are console errors on this page for the following changes:
- All uses of
class
should beclassName
- All uses of
colspan
should becolSpan
, androwspan
should berowSpan
- All the TR tags (not under THEAD) should be children of a TBODY element.
now with improved dark mode. something broke the preview though:
|
docs: Fix a couple of broken links in the docs site
Drop cosaic from docs as defunct
@kriswest and @julianna-ciq please review I've fixed all the remaining broken links and the Netlify preview is now working. I'm assuming all the items of conversation above have been fixed by your PR into this one? thanks! |
I believe the comments ended up split between suggestions above that needed
to be commited and PR for files that weren't affected/moved by your PR...
K
…On Fri, 3 Mar 2023, 11:59 Rob Moffat, ***@***.***> wrote:
@kriswest <https://github.com/kriswest> and @julianna-ciq
<https://github.com/julianna-ciq> please review
I've fixed all the remaining broken links and the Netlify preview is now
working.
I'm assuming all the items of conversation above have been fixed by your
PR into this one?
thanks!
—
Reply to this email directly, view it on GitHub
<#910 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM7PBAEQTVZNJWBWHLZWA3W2HMJBANCNFSM6AAAAAAVEHYRKQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Co-authored-by: julianna-ciq <74684272+julianna-ciq@users.noreply.github.com>
Co-authored-by: julianna-ciq <74684272+julianna-ciq@users.noreply.github.com>
Co-authored-by: julianna-ciq <74684272+julianna-ciq@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - side issue raised for content improvements
@robmoffat It doesn't look like this has deployed... See https://github.com/finos/FDC3/actions/runs/4324087758 The github action for deploying it probably also needs updating. |
I'm talking with Juan on this. I believe the action actually needs to be
removed but not completely sure right now
…On Fri, Mar 3, 2023 at 5:38 PM Kris West ***@***.***> wrote:
@robmoffat <https://github.com/robmoffat> It doesn't look like this has
deployed... See https://github.com/finos/FDC3/actions/runs/4324087758
The github action for deploying it probably also needs updating.
—
Reply to this email directly, view it on GitHub
<#910 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEK2YOC3DBWJ2OVJKZTY5TW2IUB5ANCNFSM6AAAAAAVEHYRKQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
The action needs to exist to deploy - but will no doubt need updating after
the upgrade... Unless you are suggesting moving to a manual deployment of
some sort.
…On Mon, 6 Mar 2023 at 14:09, Rob Moffat ***@***.***> wrote:
I'm talking with Juan on this. I believe the action actually needs to be
removed but not completely sure right now
On Fri, Mar 3, 2023 at 5:38 PM Kris West ***@***.***> wrote:
> @robmoffat <https://github.com/robmoffat> It doesn't look like this has
> deployed... See https://github.com/finos/FDC3/actions/runs/4324087758
>
> The github action for deploying it probably also needs updating.
>
> —
> Reply to this email directly, view it on GitHub
> <#910 (comment)>, or
> unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AAEK2YOC3DBWJ2OVJKZTY5TW2IUB5ANCNFSM6AAAAAAVEHYRKQ
>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#910 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM7PBGFXBQZWEAVDXWPPFDW2XVZDANCNFSM6AAAAAAVEHYRKQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Kris West *he • him*
*Principal Engineer*
[image: Finsemble] <https://finsemble.com/>
***@***.***
finsemble.com
|
Fixing
Major rework was required. Please review the preview carefully.