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

Shiny default graph symbols, take 2 #3499

Closed
wants to merge 3 commits into from
Closed

Conversation

algmyr
Copy link
Contributor

@algmyr algmyr commented Apr 13, 2024

I split this into a few commit to keep @thoughtpolice's initial commit intact, put my fixes on top, and shove all the tests into a separate commit to make reviewing hopefully less terrible.

Screenshot showing the new proposed style, both non-ascii and ascii style
image

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@algmyr algmyr force-pushed the algmyr/push-zvlvuqvxqwsz branch from 5e5e6f7 to aee6253 Compare April 13, 2024 01:52
@martinvonz
Copy link
Member

One option is to start with aliases as Yuya suggested in #3381 (comment). We could leave updating the default for later, after we've heard feedback from some users. That way we could potentially avoid updating all the tests multiple times. I don't feel strongly.

@joyously
Copy link

I haven't really been following the evolution of which symbols are used in the graph, but it looks like it changed the normal node from a solid circle to an empty circle. I have two concerns with that. One, which I mentioned on an issue some time back is that the empty circle and the ASCII equivalent looks like a valid change ID. Secondly, it looks so similar to @. (as does the circle with inner dot)
Why was the default changed?

@algmyr
Copy link
Contributor Author

algmyr commented Apr 13, 2024

[...] but it looks like it changed the normal node from a solid circle to an empty circle.

Nit, the previous thing wasn't a solid circle, it was a fisheye, a circle with a dot inside: .

One, which I mentioned on an issue some time back is that the empty circle and the ASCII equivalent looks like a valid change ID.

Interesting, I would have assumed that with the neutral color of the graph node compared to the change ids that wouldn't be an issue. E.g. in the default config the change id is magenta.

Secondly, it looks so similar to @. (as does the circle with inner dot) Why was the default changed?

That's also interesting, one big piece of feedback from people was that the current symbol is too similar to @ and that it's too visually heavy which makes finding the similar looking @ harder.

An image as an example with the circles for reference
image

@joyously
Copy link

the previous thing wasn't a solid circle, it was a fisheye, a circle with a dot inside

Ha ha! It looked solid to me when viewing the file changes on GitHub.
I complained to Ilya recently that the circle with a dot looked too much like @ also...
It looks different in your message where you type it than it does in the image.

I would have assumed that with the neutral color of the graph node compared to the change ids that wouldn't be an issue.

That's fine for looking at the CLI output with good vision. I was looking at code in a diff on GitHub, and there are lots of people that don't have good vision (I guess me, since I couldn't see the dot in the circle) and could be looking at a cut/paste instead of a colored output. My comment about the o that was in use when I first found this project stemmed from the density of the log output. Perhaps it has improved somewhat, or once you know what you are looking at, it's easier to read (but at first it's a mess).

@algmyr
Copy link
Contributor Author

algmyr commented Apr 13, 2024

Right, looking at the code diffs gets messy, in part because no colors, but especially since I wouldn't be surprised if the font rendering even in the monospace code looks quite different from what you might expect in a terminal. Let me put a proper screenshot that has all the symbols up top.

@algmyr
Copy link
Contributor Author

algmyr commented Apr 13, 2024

One option is to start with aliases as Yuya suggested in #3381 (comment). We could leave updating the default for later, after we've heard feedback from some users. That way we could potentially avoid updating all the tests multiple times. I don't feel strongly.

To follow up on this, I prepared this in #3501.

I'll close this for the time being since we seem to be going down the route of not shipping a new default config yet, but rather having something opt in for now.

@algmyr algmyr closed this Apr 13, 2024
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.

4 participants