ubports / telegram-app

Unofficial Telegram App for Ubuntu Desktop & Mobile
GNU General Public License v3.0
55 stars 23 forks source link

switching from Devices.density to units.gu #126

Closed mymike00 closed 6 years ago

mymike00 commented 6 years ago

for values I used 1*Devices.density equals to unis.gu(0.125)

also I changed the style of the down arrow in all the chats immagine

Flohack74 commented 6 years ago

Uh! Can you please remove all po files? First, there is no change inside despite a small timestamp change. And then, updating po files will confuse the translation homepage. They are only updated by Weblate ;)

mymike00 commented 6 years ago

oh, I didn't want to do it... if I remove those files from github and then pr, it will work?

Flohack74 commented 6 years ago

Dont delete them! :) This needs to be reverted. Deleting is not reverting but a modification ;)

mymike00 commented 6 years ago

ah, sorry I misunderstood. so how can I do it?

nfsprodriver commented 6 years ago

You may replace them by the main branch ones again (overwrite).

mymike00 commented 6 years ago

ok, now it should be ok

Flohack74 commented 6 years ago

Ok now its fine regarding this. The pixel changes are ok, too.

However, you mix in another change in this PR, which is about changing the behaviour of the sent message box, and the coloring. While basically this is a good idea, its hard to discuss 2 things in one PR. Because I can only accept both or none ;)

Thats also the reason why I keep this on hold over christmas, since we need to discuss if this change is really what we want. I agree its more like SMS then (messaging-app behaviour) but I want more opinions.

@sverzegnassi what do you think?

sverzegnassi commented 6 years ago

for values I used 1*Devices.density equals to unis.gu(0.125)

I'm not up-to-date on the current usage of those Aseman tools. But I'm very confident that Michele's assumption is correct.

On a side note, as a general advice, I would avoid to define grid units with 0.125 steps. The minimum grid unit size is 8, and that'd be in any case mathematically correct. However, Ubuntu UI Toolkit has been designed with units.dp(1) in mind for such cases.

In the PR I see e.g. radius: units.gu(0.625). That's equivalent to units.dp(5), but it'd be better written as units.dp(4) (i.e. try to use 1, 2, 4, or 8 where possible).

Between the large grid units values, I see units.gu(22.5). Those .5 gu actually stands for a 4px difference (180px vs 176px), which is very small. units.gu(22) or units.gu(24) - the latter appears in the Suru design docs - might be preferable.

sverzegnassi commented 6 years ago

I'm not aware of changes about message boxes and color usage. Could you please explain me what the change is about, @Flohack74 ?

From the diff, I would say it's about removing the ticking marks, and coloring the message box according to the message status (sending, error, sent, read).

This is an official mockup I found for the messages box design:

aa

I don't think there is any prescription in UX docs about how message status should be present to the users. UX docs should be seen as a list of "good practices", not necessarily as rules to follow. If Michele's change fits the need of telegram-app, and helps in delivering a smooth UX, then it's worth a try :)

mymike00 commented 6 years ago

However, you mix in another change in this PR

ops, I messed up some thing in this pr... :disappointed: Should I make to different PRs?

mymike00 commented 6 years ago

@sverzegnassi which is the difference between units.gu and units.dp? units.gu(1) == units.dp(8) am I right? so where should I use one instead of the other? i.e. is better units.gu(0.5) or units.dp(4)? and should I use only integer numbers when using units.gu or .dp?

sverzegnassi commented 6 years ago

@mymike00 The difference is rather conceptual. Grid units and density independent pixel do the exact same job.

What is different is the context where they are used. units.gu() is used to align items to a virtual 8px grid, while units.dp() is used to define sizes inside a 1px sub-grid. Reasonably, you might want to define radius with dp(), while all the other sizes are defined with gu(). An exception, however, might be text margin between two adjacent Label items, which might be layouted with a units.dp(2) margin. Generally speaking, however, it's fair to use gu() with 0.5 steps.

This is a distinction made to avoid gu(.125) definitions, as dp(1) is a bit more clear. But there's nothing wrong with your PR, and it doesn't affect the correct behavior of the app.

mymike00 commented 6 years ago

ok, thank you @sverzegnassi , now it's all clear. But I didn't understand if I have to make 2 different pull requests for unit measure changes and for graphic improvements...

Flohack74 commented 6 years ago

Lets say in this case leave it like it is, but for future, please split topics across different PRs :)

mymike00 commented 6 years ago

ok, I was sure I didn't pull the latest changes (messages colors) to GitHub... sorry again :cry:

mymike00 commented 6 years ago

so it should be better if I change everything you suggested? why didn't you do by yourself instead of comment it?

sverzegnassi commented 6 years ago

so it should be better if I change everything you suggested? why didn't you do by yourself instead of comment it?

Sorry if I see your comment right now, on 29/12 i was going to be afk for an entire week.

I've been suggesting not to use .125 gu steps in size definitions, so I pointed out where those .125 steps were still used. The PR was already valid - indeed I approved your changes even with those values. It was a 'non-mandatory' request, in case you were willing to do such changes.

I left a further note - which meets @Flohack74 's interest too - about refreshing telegram-app visuals in future, since they don't perfectly match Suru specifications. I guess that was also the reason why Flo asked my review here.

Anyway, I think there is no further change required for this PR.

@Flohack74 - LGTM do you think it's ready for a merge?

mymike00 commented 6 years ago

ok, no problem. I agree with you the app needs a bit of restyling, but where can I read more about Suru specifications?

Flohack74 commented 6 years ago

Yeah lets go!

mymike00 commented 6 years ago

I think we had to tag the issue #69 here...