-
Notifications
You must be signed in to change notification settings - Fork 60
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
new feature list #101
new feature list #101
Conversation
488e15f
to
785cee8
Compare
ce6ea67
to
a923c80
Compare
That looks brilliant already. So clean! :) I have a few nits.
|
That looks brilliant already. So clean! :) I have a few nits.
|
Thanks! With the labels -- I want to make sure they do not distract from higher importance information. Also maybe they need a hover text to explain what they are.. Maybe we can increase the opacity. Agree with the rest |
Another suggestion would be to remove the background color and increase the border-size ( with the full sat color) and just have the full saturation on hover. |
I addressed three of the nits
However, after I went through all of the comments and tried them, I realized that they should be moved out into separate issues, and that they need more discussion. I would like to say that we can always iterate. Some less contentious items:
Moving this to the right pulls it out of the flow of reading. I tried this while iterating on it, and I found I would frequently miss this label if I was looking for examples. I agree that it is not very visible, so I made it bold and underlined. Adding padding is a bit tricky, the goal here is to compress information. This should be iterated on in a separate issue. This is what it looks like now: I am not super happy about it, combined with the bold authors, because now we have a lot of weird stuff with bold going on in the text. I would prefer to use one font weight here. But I do not want to spend much more time on this and if we want to change this, precise, actionable feedback is preferred.
With regards to the animation, this is the animation we had before and I didn't change it. I would say that should we want to iterate on it, it should be moved into a separate bug. I did try it, but it did not look very good making this change and should have more time dedicated to it. Otherwise, there are some items which, after implementing them, I realized I do not agree with or perhaps didn't understand. They should be moved out into their own issues, if we want to pursue them, however I have strong feelings against them after seeing them. Specifically:
My goal with this change was to make this stand out, removing the bottom border makes it almost blend in with the background and look unintentional: Removing the white background likewise has a similar effect: Since one of the major requests was that this bar stood out more, I think that it should stay as it is in this pr. At least for now, we can revisit it once we have more time.
I checked the contrast and there is no readability issue. Having every text area in a white background will reduce importance of the main navigation, and makes the visual layout very busy. Suggested change: As an alternative we can have it all as a single white space, but I do not find that we have any real improvement in attention here. Let's discuss these separately |
After talking with ola offline, we agreed to merge and follow up in separate bugs |
stuff that still needs to be done:
high prio
Low prio