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.

Sign up for GitHub

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

Jump to bottom

fix see more #464

Merged
merged 1 commit into from
Mar 4, 2024
Merged

fix see more #464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions app/components/Article/Contents.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,17 @@ const Contents = ({pageid, html, glossary}: {pageid: PageId; html: string; gloss

updateTextNodes(el, insertGlossary(pageid, glossary))

const toggleClass = (e: Event) => {
e.preventDefault()
const target = e.target as HTMLElement
target.classList.toggle('visible')
}
Array.from(document.getElementsByClassName('see-more')).forEach((e) => {
const elem = e.cloneNode(true)
elem.addEventListener('click', toggleClass)
e.replaceWith(elem)
})

// In theory this could be extended to all links
el.querySelectorAll('.footnote-ref > a').forEach((e) => {
const footnote = footnoteHTML(el, e as HTMLAnchorElement)
Expand Down
2 changes: 1 addition & 1 deletion app/server-utils/parsing-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export const convertToHtmlAndWrapInDetails = (markdown: string): string => {
// On the other hand, if the whole text isn't rendered as one, footnotes will break.
// To get round this, replace the [See more...] button with a magic string, render the
// markdown, then mangle the resulting HTML to add an appropriate button link
markdown = markdown.replaceAll(/\s*\[[Ss]ee more\W*?\]\s*/g, seeMoreToken)
markdown = markdown.replaceAll(/\[[Ss]ee more\W*?\]/g, seeMoreToken)
markdown = md.render(markdown)
markdown = wrap(markdown.split(seeMoreToken))

Expand Down
17 changes: 13 additions & 4 deletions app/server-utils/stampy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export type AnswersRow = CodaRowCommon & {
Banners: '' | Entity[]
'Rich Text': string
Subtitle?: string
Icon?: string
Icon?: Entity | string | Entity[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This type change doesn't seem necessary as far as I can tell. The new extractIcon function returns string | undefined, so I think that is a suitable type here.

Suggested change
Icon?: Entity | string | Entity[]
Icon?: string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is here mainly to document what Coda returns. Which can be either the raw image (base64 encoded), an url to the image, a reference to a Coda item or a list of references to Coda items... I'm not sure what exactly causes what

Parents?: Entity[]
Order?: number
}
Expand Down Expand Up @@ -250,7 +250,7 @@ const renderText = (pageid: PageId, markdown: string | null): string | null => {
if (!markdown) return null

markdown = extractText(markdown)
markdown = urlToIframe(markdown)
markdown = urlToIframe(markdown || '')

let html = convertToHtmlAndWrapInDetails(markdown)
html = uniqueFootnotes(html, pageid)
Expand All @@ -260,8 +260,17 @@ const renderText = (pageid: PageId, markdown: string | null): string | null => {
return html
}

// Icons can be returned as strings or objects
const extractIcon = (val?: string | string[] | Entity | Entity[]): string | undefined => {
if (!val) return val
const item = head(val)
if (typeof item === 'string') return extractText(val as string)
if (item) return (item as Entity).url
return undefined
}

Comment on lines +266 to +271
Copy link
Collaborator

@jrhender jrhender Mar 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would be nice to avoid the val as casts to benefit from type checking of val in the future. I've suggested an isStringOrStringArray type guard to do this, though there might be a more elegant way.

Suggested change
const item = head(val)
if (typeof item === 'string') return extractText(val as string)
if (item) return (item as Entity).url
return undefined
}
if (isStringOrStringArray(val)) return extractText(val)
const item = head(val)
if (item) return item.url
}
const isStringOrStringArray = (x: unknown): x is string | string[] => {
const item = head(x)
return typeof item === 'string'
}

I also think the last return undefined isn't necessary. I think undefined will be returned if none of the if cases are matched, even without this line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the thing is that I know that if it's not a string and not undefined, then it's going to be an Entity at this point. Typescript doesn't know that.

Wouldn't that be void?

Copy link
Collaborator

@jrhender jrhender Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the thing is that I know that if it's not a string and not undefined, then it's going to be an Entity at this point. Typescript doesn't know that.

Ah maybe part of the confusion here is that in my local code where I was trying this out, I also changed the signature of the head function to const head = <T,>(item: T | T[]): T. With this, I'm pretty sure that typescript does know that item is an Entity at that point. You can see in screenshot below that my local compiler is showing the type of item as that point as Entity.

image

The way I'm reasoning about it is that the parameter type is undefined | string | string[] | Entity | Entity[] and then:

  1. if (!val) return val removes the possibility of undefined
  2. if (isStringOrStringArray(val)) return extractText(val) removes the possibility of string | string[]
  3. head (with the signature change above) narrows Entity | Entity[] to Entity

At any rate, I'll make a PR at some point to improve the head function so that it's easy to make future code more type safe 😊 . Probably less important getting the UI redesign out.

Wouldn't that be void?

No, I don't think so. I think void is a typescript type but the actual return type is undefined. I wrote this JS fiddle as a quick test: https://jsfiddle.net/#&togetherjs=M9fHnDlskK

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ohhh, I like that version of head!

// Sometimes string fields are returned as lists. This can happen when there are duplicate entries in Coda
const head = (item: string | string[]) => {
const head = (item: any | any[]) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it would be more type-safe down the line to use unknown here. This change doesn't seem to break anything in the existing code and so should be easy to apply now.

Suggested change
const head = (item: any | any[]) => {
const head = (item: unknown | unknown[]) => {

if (Array.isArray(item)) return item[0]
return item
}
Expand Down Expand Up @@ -291,7 +300,7 @@ const convertToQuestion = ({name, values, updatedAt} = {} as AnswersRow): Questi
status: values['Status']?.name as QuestionStatus,
alternatePhrasings: extractText(values['Alternate Phrasings']),
subtitle: extractText(values.Subtitle),
icon: extractText(values.Icon),
icon: extractIcon(values.Icon),
parents: !values.Parents ? [] : values.Parents?.map(({name}) => name),
updatedAt: updatedAt || values['Doc Last Edited'],
order: values.Order || 0,
Expand Down
Loading