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

Adding docs for upcoming inlineMessage component #746

Merged
merged 9 commits into from
Jun 12, 2024
Merged

Adding docs for upcoming inlineMessage component #746

merged 9 commits into from
Jun 12, 2024

Conversation

lukasoppermann
Copy link
Contributor

@lukasoppermann lukasoppermann commented Mar 22, 2024 •
edited
Loading

Waiting for primer/react#4443 to merge & publish

@lukasoppermann lukasoppermann requested a review from a team as a code owner March 22, 2024 09:42
@lukasoppermann lukasoppermann marked this pull request as draft March 22, 2024 09:42
@lukasoppermann lukasoppermann self-assigned this Mar 22, 2024
mperrotti
mperrotti reviewed Mar 22, 2024
View reviewed changes
Copy link
Contributor

@mperrotti mperrotti left a comment

Choose a reason for hiding this comment

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

These are looking good!

My only feedback is about phrasing and formatting.

content/components/inline-message.mdx Show resolved Hide resolved
content/components/inline-message.mdx Outdated Show resolved Hide resolved
content/components/inline-message.mdx Outdated Show resolved Hide resolved
content/components/inline-message.mdx Outdated Show resolved Hide resolved
content/components/inline-message.mdx Outdated Show resolved Hide resolved
content/components/inline-message.mdx Outdated Show resolved Hide resolved
content/components/inline-message.mdx Show resolved Hide resolved
src/@primer/gatsby-theme-doctocat/nav.yml Outdated Show resolved Hide resolved
content/components/inline-message.mdx Outdated Show resolved Hide resolved
content/components/inline-message.mdx Outdated Show resolved Hide resolved
@mperrotti mperrotti mentioned this pull request Mar 22, 2024
Merged
@lukasoppermann @mperrotti
Apply suggestions from code review
854ef26 
Co-authored-by: Mike Perrotti <mperrotti@github.com>
lukasoppermann and others added 3 commits March 26, 2024 15:04
@lukasoppermann @mperrotti
Update content/components/inline-message.mdx
0f8aca4 
Co-authored-by: Mike Perrotti <mperrotti@github.com>
@lukasoppermann @mperrotti
Update content/components/inline-message.mdx
2718e25 
Co-authored-by: Mike Perrotti <mperrotti@github.com>
@lukasoppermann @mperrotti
Update content/components/inline-message.mdx
404fedc 
Co-authored-by: Mike Perrotti <mperrotti@github.com>
joshblack
joshblack reviewed Mar 27, 2024
View reviewed changes
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Just wanted to say looks great! 🔥

One question I had was for message contents, should these be text-only or is it okay for them to have interactive content?

Also wanted to ask if it is intended for the inline messages to be live region announcements?

@joshblack joshblack mentioned this pull request Jun 6, 2024
Merged
11 tasks
@langermank
Copy link
Contributor

@lukasoppermann can you add the size option? We have small and medium (default)

  • default 14px, outline icon
  • small 12px, filled icon
image

Then I think this is good to go once Josh merges the React PR 😄

@lukasoppermann
Copy link
Contributor Author

lukasoppermann commented Jun 11, 2024 •
edited
Loading

@joshblack

Just wanted to say looks great! 🔥
One question I had was for message contents, should these be text-only or is it okay for them to have interactive content?
Text only for now. Since inline messages are supposed to be directly where the related content is, I currently don't see any need for links.

Also wanted to ask if it is intended for the inline messages to be live region announcements?
@khiga8 or @ichelsea can you help here?

@lukasoppermann lukasoppermann marked this pull request as ready for review June 11, 2024 11:39
@lukasoppermann lukasoppermann merged commit eca609d into main Jun 12, 2024
6 checks passed
@lukasoppermann lukasoppermann deleted the inlineMessage branch June 12, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshblack joshblack joshblack left review comments

@mperrotti mperrotti mperrotti left review comments

@dipree dipree Awaiting requested review from dipree

@langermank langermank Awaiting requested review from langermank

Assignees

@lukasoppermann lukasoppermann

Labels
area: documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lukasoppermann @langermank @mperrotti @joshblack