Skip to content

[New icon] Request changes icon#267

Closed
katmeister wants to merge 3 commits intomasterfrom
request-changes-icon
Closed

[New icon] Request changes icon#267
katmeister wants to merge 3 commits intomasterfrom
request-changes-icon

Conversation

@katmeister
Copy link
Copy Markdown
Contributor

As part of the friendlier requested changes Pioneer project, I'm proposing a softened requested changes code review icon to replace the x + red circle combo we have now!

Relevant links:

icons-sizes

@primer primer deleted a comment from figma-diff Bot Dec 12, 2018
@jonrohan
Copy link
Copy Markdown
Member

Thanks @katmeister love it.

@sophshep
Copy link
Copy Markdown

Love this @katmeister!!

@joelhawksley
Copy link
Copy Markdown
Contributor

@jonrohan @sophshep @shawnbot might we be able to get this shipped today? We'd like to ship the feature that uses this icon on Monday.

@katmeister
Copy link
Copy Markdown
Contributor Author

Some recreated screenshots/mocks of how this looks in context:

mergebox-review

reviewers-list

@jonrohan
Copy link
Copy Markdown
Member

jonrohan commented Jan 4, 2019

Two things, I don't think they're icon specific.

  1. The icon reads high in the circle
  2. I don't think making it red is necessary

@emilybrick
Copy link
Copy Markdown
Contributor

Ah, nice! The context is def helpful.

I do actually think making it red is helpful, to some extent, to indicate that this is something the user needs to respond to. It does read a tad high in the circle, not sure if that's just optical or not.

@broccolini
Copy link
Copy Markdown

The icon reads high in the circle

Agree on this. Has the alpha release been tested in dotcom? That would be a more robust way of testing the icon in all it's uses. It looks like the icon is vertically centered according to the total height of the icon, but feels off because of the small tail on the speech bubble when it's inside a circle, so perhaps this needs to visually centered.

I don't think making it red is necessary

I was also wondering whether changing the icon alone will address users concerns and make a real impact to how these comments are received. If it still can't be shipped until the requested changes have been addressed then red feels like the right color, if it can be ignored (depending on settings) then red might not be the right color. I think this is up to the project team though and will be an interesting test to see if it makes a difference.

Feedback on the icon design: The weight of the icon feels off compared to the other icons—the warning icon is filled in so it feels heavier, the checkmark is simpler and has thicker lines. The new icon is quite detailed in comparison. My only thought was whether this would work without the surrounding speech bubble? I think the +- communicates changes alone, and some of the other icons as part of a comment (i.e. approved) but we don't put them in speech bubbles. Again I think testing in dotcom so that you see it in real scenarios would be helpful.

I am not suggesting this is a blocker though, it's difficult to get the balance right when we have lots of inconsistencies in weights and styles across Octicons, so up to you.

@primer primer deleted a comment from figma-diff Bot Jan 7, 2019
@primer primer deleted a comment from figma-diff Bot Jan 7, 2019
@lukehefson
Copy link
Copy Markdown

I don't think making it red is necessary

[…] I think this is up to the project team though and will be an interesting test to see if it makes a difference.

We'd really like to stick with red as it is important for it to be able to denote: "blocking". (FYI: other ways we can denote blocking have been much discussed and we've always ended up circling back around to red)

@jonrohan
Copy link
Copy Markdown
Member

jonrohan commented Jan 8, 2019

This will be part of 8.3.0 #273

@jonrohan jonrohan closed this Jan 8, 2019
@jonrohan jonrohan deleted the request-changes-icon branch January 8, 2019 22:34
@jonrohan jonrohan mentioned this pull request Jan 8, 2019
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.

8 participants