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.

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

docs(canvas): add support note for Braille marker #472

Merged
merged 1 commit into from
Sep 10, 2023
Merged

Conversation

joshka
Copy link
Member

@joshka joshka commented Sep 7, 2023

Addresses #457

@joshka
Copy link
Member Author

joshka commented Sep 7, 2023

Pinging @relf for input

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #472 (73d57ae) into main (0c68ebe) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #472   +/-   ##
=======================================
  Coverage   90.06%   90.06%           
=======================================
  Files          40       40           
  Lines       11166    11166           
=======================================
  Hits        10057    10057           
  Misses       1109     1109           
Files Changed Coverage Δ
src/symbols.rs 72.41% <ø> (ø)
src/widgets/canvas/mod.rs 91.90% <ø> (ø)

@Valentin271
Copy link
Member

I wonder should we link to some fonts that support this? Otherwise LGTM.

Copy link
Member

@orhun orhun left a comment

Choose a reason for hiding this comment

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

+1

@relf
Copy link

relf commented Sep 7, 2023

The thing is, discovering ratatui, I just ran the demo and seing I did not get the right result out of the box, I started to dive into the code to find out, your additions would have definitely helped.
That said, I've just noticed now (!) that I've skipped the warning just after in the example section where you explain about having gliphs not showing up correctly 🤦‍♂️, maybe some bold font could have helped (?) and/or links to "good" fonts would be nice right here.
The default configuration on Windows (Consolas font) does not work, I think this could be stressed as an example of "bad" configuration.
Anyway thanks and keep up the good work!

@kdheepak
Copy link
Collaborator

kdheepak commented Sep 7, 2023

Should we also add a one line text description to the ui in the example for chart.rs?:

https://github.com/ratatui-org/ratatui/blob/0c68ebed4f63a595811006e0af221b11a83780cf/examples/chart.rs#L145

Something like "Requires terminal and font support for unicode".

@joshka
Copy link
Member Author

joshka commented Sep 8, 2023

The default configuration on Windows (Consolas font) does not work, I think this could be stressed as an example of "bad" configuration.

I'd be more inclined to suggest that whatever we do with demos should generally look good out of the box for a default configuration, so perhaps we should switch to dots instead of braille for the main demo and introduce the braille markers later?

@Valentin271
Copy link
Member

Yes this seems like a good idea to me.

@relf
Copy link

relf commented Sep 8, 2023

demos should generally look good out of the box for a default configuration, so perhaps we should switch to dots instead of braille for the main demo and introduce the braille markers later

Yes that was my first thought but I did not dare to suggest it (more involved changes), but now that you say it...

@joshka
Copy link
Member Author

joshka commented Sep 10, 2023

I wonder should we link to some fonts that support this? Otherwise LGTM.

Perhaps a wiki page might be appropriate for this?

@joshka joshka merged commit 17797d8 into main Sep 10, 2023
31 checks passed
@joshka joshka deleted the docs-braille branch September 10, 2023 00:08
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.

5 participants