Skip to content

Adjust alert/aside design #3634

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

Merged
merged 3 commits into from
Jan 24, 2024
Merged

Conversation

parlough
Copy link
Member

@parlough parlough commented Jan 23, 2024

Makes some adjustments to:

  • More closely match developer's expectations from GitHub alerts
  • Reduce spacing for to better align with upcoming changes to dart.dev's alerts
  • Increase contrast of alert text in dark mode
  • Add a unique icon and color for each type

Dark mode:

New dark mode styles

Light mode:

New light mode styles

@parlough parlough changed the title Adjust alert aside design Adjust alert/aside design Jan 23, 2024
@parlough
Copy link
Member Author

\cc @devoncarew

Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

I'm assuming you'd seen my redux of the initial css - #3628.

More closely match developer's expectations from GitHub alerts

I do prefer the in-line title and first paragraph style, but

Reduce spacing for to better align with upcoming changes to dart.dev's alerts

I think aligning w/ something else in our space is more important. Do you have screenshots of the upcoming design?

Increase contrast of alert text in dark mode

👍

Add a unique icon and color for each type

👍 - I hadn't worked out the css-foo to prefix the icons

@parlough
Copy link
Member Author

parlough commented Jan 23, 2024

I'm assuming you'd seen my redux of the initial css - #3628.

Yep, thanks for adding support for these and aligning the styles with dart.dev!

I do prefer the in-line title and first paragraph style, but

Yeah we've used it for a while and I've gotten used to it, but after canvasing other related documentation sites and tooling, I found the separated title is quite a bit more common. I decided it's likely better to be familiar. Especially in this case where it's a syntax specifically from GitHub's markdown, which users may expect us to more closely align with.

After trying it out for a bit, I found it makes it easier to identify the type of alert. It's potentially easier to localize/translate as well, but that's not super relevant for the API docs currently :P

Do you have screenshots of the upcoming design?

It's a minor change, pretty much just to separate the title and reduce padding:

Example of new dart.dev design

I'm happy to make changes though (here and on dart.dev)! I don't feel tied to any design or style of these as the important change here is the improved contrast. Whatever we end up with here, I'll try to align with that on dart.dev :)

Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

I'm happy to make changes though (here and on dart.dev)! I don't feel tied to any design or style of these as the important change here is the improved contrast. Whatever we end up with here, I'll try to align with that on dart.dev :)

I feel like you guys should own the styles, and general alignment between dartdoc and dart.dev. lgtm :)

Copy link
Member

@srawlins srawlins left a comment

Choose a reason for hiding this comment

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

Love it!

@parlough
Copy link
Member Author

Thanks both of you for the review!

Feel free to merge when ready. I don't have access on this repository.

@srawlins srawlins merged commit 80ef7a8 into dart-lang:main Jan 24, 2024
@parlough parlough deleted the feat/alert-redesign branch January 24, 2024 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants