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

Remove border radius from icon-toggle-pictures #20539

Closed
wants to merge 1 commit into from
Closed

Conversation

marcoambrosini
Copy link
Member

@marcoambrosini marcoambrosini commented Apr 17, 2020

Screenshot from 2020-04-21 10-54-30

@skjnldsv
Copy link
Member

skjnldsv commented Apr 17, 2020

So, you used a radius of 1, but be aware that the grid one have a radius of .5 :)

<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewbox="0 0 16 16" width="16" height="16">
	<rect rx=".5" ry=".5" height="6" width="6" y="1" x="1"/>
	<rect rx=".5" ry=".5" height="6" width="6" y="1" x="9"/>
	<rect rx=".5" ry=".5" height="6" width="6" y="9" x="9"/>
	<rect rx=".5" ry=".5" height="6" width="6" y="9" x="1"/>
</svg>

@marcoambrosini
Copy link
Member Author

marcoambrosini commented Apr 17, 2020

be aware that the grid one have a radius of .5 :)

Can I remove all this nearly invisible border radiuses and use straight squares here? It would help a lot also with how the 3 little squares look..

@skjnldsv
Copy link
Member

Can I remove all this nearly invisible border radiuses and use straight squares here? It would help a lot also with how the 3 little squares look..

see with Jan, I'm fine with whatever :p

@jancborchardt
Copy link
Member

be aware that the grid one have a radius of .5 :)

Can I remove all this nearly invisible border radiuses and make squares here? It would help a lot also with how the 3 little squares look..

If it looks good, go ahead. :)

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks super nice! Seeing how we now have 3 icons without text next to each other, shouldn’t we get the 3-dot menu?

Start call button | 3-dot menu | Sidebar icon

"Fullscreen" and "Grid view" (shorter wording btw) can be in the 3-dot menu. If we want to, we can even put the sidebar icon in the 3-dot menu as "Participants & settings" or just "Conversation settings".

@marcoambrosini
Copy link
Member Author

Looks super nice! Seeing how we now have 3 icons without text next to each other, shouldn’t we get the 3-dot menu?

I wouldn't do this Jan because we are going to bring the whole moderation menu as a 3 dot menu in the app content. Plus those 2 controls are very important for the call imo: provided there's enough width, I think we should show them explicitly

@marcoambrosini
Copy link
Member Author

@skjnldsv do you know why the build is failing?

@skjnldsv
Copy link
Member

Unrelated

@nickvergessen
Copy link
Member

I would vote that these are very specific icons and they should just go to the Talk app for now.

Also they should be minified hiding the script in build/

@marcoambrosini
Copy link
Member Author

these are very specific icons

Only the promoted-view icon is specific to talk, the grid is used in other apps too.

@rullzer rullzer mentioned this pull request Apr 18, 2020
55 tasks
@rullzer
Copy link
Member

rullzer commented Apr 18, 2020

these are very specific icons

Only the promoted-view icon is specific to talk, the grid is used in other apps too.

fair enough. Then move the promoted view to talk ;)

@marcoambrosini
Copy link
Member Author

marcoambrosini commented Apr 18, 2020

Wouldn't be better, for the sake of standardization, to leave it here? I mean, that icon is in the end just 1 big rectangle on top of 3 smaller rectangles, it could also be easily a gallery icon, or something else we haven't thought of! What if a contributor developing an app needs something like this? Isn't it better to provide it in the server instead of having him create his own version?

Edit: we could also add it to talk for now to simplify backports, but I would leave a version of it here as well.

@rullzer
Copy link
Member

rullzer commented Apr 19, 2020

Wouldn't be better, for the sake of standardization, to leave it here? I mean, that icon is in the end just 1 big rectangle on top of 3 smaller rectangles, it could also be easily a gallery icon, or something else we haven't thought of! What if a contributor developing an app needs something like this? Isn't it better to provide it in the server instead of having him create his own version?

Edit: we could also add it to talk for now to simplify backports, but I would leave a version of it here as well.

Just dumping all icons in server is not a solution IMO.
Apps should provide their own icons as much as possible. If not only for the backports. And making sure the icon in server isn't suddenly changed.

If it turns out an icon is used consistently across apps for function X. Then we can always move it to server. Or as part of the vue components or whatever.

But lets do that once it is needed. If some app comes along now and thinks they needit. They can just copy the icon from talk.

Let try to keep the server as clean as we can. And the apps as self contained as we can.

@rullzer
Copy link
Member

rullzer commented Apr 19, 2020

Random brain fart. I don't know how easy/feasible it is. But maybe some npm packages nextcloud/icons would make sense?

That way we still have all the standardized icons. But apps just pick the ones they need?

@skjnldsv
Copy link
Member

Random brain fart. I don't know how easy/feasible it is. But maybe some npm packages nextcloud/icons would make sense?

this is planned™ ^^

@marcoambrosini
Copy link
Member Author

Allrighty!

@marcoambrosini marcoambrosini force-pushed the newicons branch 2 times, most recently from b3928a4 to a126931 Compare April 20, 2020 06:30
@marcoambrosini marcoambrosini changed the title Add promoted-view and square icons Remove border radius from icon-toggle-pictures Apr 20, 2020
@marcoambrosini
Copy link
Member Author

Done, the only change is now the modification to icon-toggle-pictures

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

See comment, please also update the main post screenshots so people know what it is about now ;)

core/img/actions/toggle-pictures-white.svg Outdated Show resolved Hide resolved
@skjnldsv skjnldsv added the 2. developing Work in progress label Apr 20, 2020
@skjnldsv skjnldsv added design Design, UI, UX, etc. enhancement labels Apr 20, 2020
@jancborchardt
Copy link
Member

But lets do that once it is needed. If some app comes along now and thinks they needit. They can just copy the icon from talk.

Agreed on that. No premature standardization. :)

Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Wait a sec – the border-radius removal we actually don’t want. The rounded style is something we go for across the design, and the corners should stay softened as they are now. Feel free to increase the radius to 1px for example.

@marcoambrosini
Copy link
Member Author

marcoambrosini commented Apr 21, 2020

@jancborchardt please take a look at these icons at 100% . There's no roundness to be seen neither with .5 nor with 1px border radius. We're just making these icons blurry.. It's true that we go for the rounded style but there's no need to do this with icons too imo, and many other icon have straight edges:

Screenshot from 2020-04-21 14-38-03

@rullzer rullzer mentioned this pull request Apr 23, 2020
11 tasks
@jancborchardt
Copy link
Member

jancborchardt commented Apr 23, 2020

@ma12-co I agree that currently it’s inconsistent and this is something we need to work on. But look at the latest icons: details/info, confirm, comment, fullscreen, menu-sidebar, video-off, audio-off.

All of these are rounded or have a majority of rounded elements in them, which is what we are going for, and what is absolutely a difference to them being squared, even on icon size. The others merely need to be adjusted, and we shouldn’t take steps back.

(And sorry for the confusion: My review above was mostly for when this issue was about the promoted view icon.)

@jancborchardt jancborchardt removed this from the Nextcloud 19 milestone Apr 23, 2020
@marcoambrosini
Copy link
Member Author

:cough:

@marcoambrosini marcoambrosini deleted the newicons branch April 23, 2020 08:21
@jancborchardt
Copy link
Member

:cough:

We are all for a unified style – for that reason it seems off to remove the rounding which is intentional (and visible) from only 1 icon, no?

Yes, Material Design has an icon set where they have sharp and rounded corners mixed. Let’s look at others as well:

  • SF Symbols from Apple: Uses a lot of rounding of the corners to make their style more approachable, friendly and easily perceivable. (They also round their hardware and app icons as we know.)
  • Bootstrap icons: Even more rounding there
  • Fontawesome: I don’t particularly like their specific style, but they also round stuff off.

@skjnldsv
Copy link
Member

  • Fontawesome: I don’t particularly like their specific style, but they also round stuff off.

should be avoided, they changed license a while ago iirc, Forkawesome is the new OSS version

@marcoambrosini
Copy link
Member Author

marcoambrosini commented Apr 23, 2020

@jancborchardt the cough was more of a "Let's use these instead" :)
They have rounded versions too!
https://material.io/resources/icons/?style=round

@marcoambrosini
Copy link
Member Author

Discussion continues here: nextcloud-libraries/nextcloud-vue#1051

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress design Design, UI, UX, etc. enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants