feat(Message): Add support for footnotes#648
feat(Message): Add support for footnotes#648rebeccaalpert merged 8 commits intopatternfly:mainpatternfly/chatbot:mainfrom
Conversation
|
Preview: https://chatbot-pr-chatbot-648.surge.sh A11y report: https://chatbot-pr-chatbot-648-a11y.surge.sh |
a54f1dd to
071d590
Compare
071d590 to
7c29806
Compare
kaylachumley
left a comment
There was a problem hiding this comment.
no specific design notes! wasn't sure if the footnote text should start inline with the numbers instead of on the line below the number? i'll leave that up to erin!
edonehoo
left a comment
There was a problem hiding this comment.
a couple thoughts on the content in the example!
Do we have any control over the return to original text arrows?
If so, I feel like the use of a footnote style there too is a little confusing -- since the second arrow has a "2" footnote, but really just links to the 2nd occurrence of footnote 1. I can offer more specific suggestions if others agree and it is something we can customize, but I couldn't tell
packages/module/patternfly-docs/content/extensions/chatbot/examples/Messages/BotMessage.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/Messages/BotMessage.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/Messages/BotMessage.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/Messages/BotMessage.tsx
Outdated
Show resolved
Hide resolved
packages/module/patternfly-docs/content/extensions/chatbot/examples/Messages/BotMessage.tsx
Outdated
Show resolved
Hide resolved
We could hook into the "data-footnote-backref" class that gets applied by the Markdown -> HTML conversion to hide superscripts ( We get what's there largely for free from https://github.com/remarkjs/remark-gfm, which is based on GitHub's defaults. |
thatblindgeye
left a comment
There was a problem hiding this comment.
Some general comments in testing out the examples:
- Not sure we should visually hide the "Footnotes" heading. Visually having it present might help make it immediately clear what that message contains, whereas if there's a lengthy message with several footnote links, when you finally get to the message containing the footnotes content the connection might be lost.
- Are consumers able to customize the content of the footnotes headding and id? We should alloww for that in the case of several footnote messages where maybe the headings should be "Footnotes for Message from 8:05"/"Footnotes for [some synopsis of the convo)"
- Also not sure about the background color of the message changing when focus is moved when clicking a footnote link - might be a bit unexpected?
- For the "User" messages, clicing the "1" footnote in the first message works as expected. Clicking the "2", however, focus gets lost. Then when clicking the "1" in the second message works as expected, but clicking the link to "go back" moves focus to the first message - I would expect focus to go to the second message's "1" footnote link.
- The "go back" links in the footnote content is also a bit iffy - clicking the link with a "2" in it brings me back to the second message "1" footnote link, and the last "go back" link brings me to the first messages "2" footnote link
- You had mentioned scrolling in the smart scroll demo, not sure if that comment included other examples like the basic "User messages" example. In that the page doesn't scroll so that the focused item is in view - that's something we should resolve even if it's only in example docs.
- Can we include some verbiage about footnote links with the same content should link to the same thing? I.e. the "1" footnote links should both go to the same footnote content item, and "go back" links similarly similarly should go back to the same footnote link?
- The labeling of the "go back" links seems a little off? "Back to reference 1" is fine, but then the next one with a "2" visually is "Back to reference 1 - 2", and the last one (which looks the same as the first one) says "Back to reference 2"
- The focus logic when going from a "go back" link to a footnote link is mostly as I'd expect, but I'm not sure if focus should go to the "go back" link when clicking a footnote link. That may not allow a user to easily digest the footnote content without having to navigate backwards. Might be better to give a -1 tabindex on the
liand programmatically focus that when clicking a footnote link
Not exactly sure how much of this we can easily fix, but I think a lot of it is stuff we need to resolve even if it's example-only code before shipping implementations.
621518b to
f1de039
Compare
|
Users should be able to adjust headers/ids using any of these options here: https://github.com/remarkjs/remark-rehype?tab=readme-ov-file#fields Example on a Message: 
|
6b93a95 to
1b029f3
Compare
a1af692 to
789af1d
Compare
|
Addressed style changes we discussed @kaylachumley @thatblindgeye @edonehoo! |
c623d55 to
44c00a8
Compare
|
Just rebasing. |
Styled footnotes and added demos with some onClick events. Assisted-by: Cursor (used for debugging demos, copying bot work to user message demo, and removing node from all components except table)
Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com>
3360bc8 to
8a11eca
Compare
8a11eca to
ae6eabf
Compare
There was a problem hiding this comment.
Overall I think this is looking great. Just a couple of comments:
- Personally still unsure about the background color change when clicking a footnote, but I'm fine keeping it and us keeping an eye/ear out for feedback on it. Mainly worried of it being a visual affordance we don't really employ elsewhere (or at least I haven't come across it I guess, wouldnt be surprised if it's common outside core React). And if this isn't something we have (easy) control over that's understandable as well
- The second "back to" button in the user messages example is fairly wide compared to the Bot Message example:
- The id's on the footnote headers in both examples are the same so that should be resolved if we can easily do that right now. Otherwise we can open a followup if it'd be a little more involved
thatblindgeye
left a comment
There was a problem hiding this comment.
We're gonna open follow ups as needed for the above, otherwise this lgtm!
|
See follow-ons:
We'll adjust the highlighting if/when products start using footnotes to match what's actually being done. No one's actually using these yet and we just wanted to demo some sort of interaction even if it's not necessarily 100% perfect. |
|
🎉 This PR is included in version 6.4.0-prerelease.20 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Styled footnotes and added demos with some onClick events. Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com> Assisted-by: Cursor (used for debugging demos, copying bot work to user message demo, and removing node from all components except table)
Styled footnotes and added demos with some onClick events. Used highlighting rather than scrolling - let me know if you have feelings on this.
Tested click behavior function for messages in main ChatBot demo as well and it worked ok in basic demo.
In smart scroll demo, got it to work with small mods (not shipping):
const footnoteNavigationActive = React.useRef(false);if (!messageBoxRef.current?.isSmartScrollActive() || scrollQueued.current || footnoteNavigationActive.current) {if (shouldUpdate && !footnoteNavigationActive.current) {Did not perfect scrolling in smart scroll demo - just used default focus without preventScroll. I figure this would be up to products what they want to do/not all may want to use backrefs this way.
Assisted-by: Cursor (used for debugging demos, copying bot work to user message demo, and removing node from all components except table -- these were getting passed to the DOM as objects and cluttering things up)
Demos (just pick "footnote" from select):