Skip to content

Conversation

@triallax
Copy link
Contributor

@triallax triallax commented Mar 1, 2021

Follow-up to #1387.

Guidelines: https://material.io/design/usability/bidirectionality.html#mirroring-elements

I'm not an expert on this topic, so it would be great if someone more knowledgeable about this would take a quick look and see if I made any mistake.

@triallax
Copy link
Contributor Author

triallax commented Mar 1, 2021

Should the left and right arrows be mirrored too? I only mirrored the back arrow.

@triallax triallax changed the title Automirror some icons according to guidelines Auto-mirror some icons according to guidelines Mar 1, 2021
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #1391 (20862d0) into main (c2220e5) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1391   +/-   ##
=======================================
  Coverage   13.77%   13.77%           
=======================================
  Files         505      505           
  Lines       21622    21622           
  Branches     3969     3969           
=======================================
  Hits         2978     2978           
+ Misses      17894    17893    -1     
- Partials      750      751    +1     
Impacted Files Coverage Δ
...oroo/astrid/adapter/CaldavManualSortTaskAdapter.kt 73.68% <0.00%> (-10.53%) ⬇️
...ava/com/todoroo/astrid/gtasks/GtasksListService.kt 80.00% <0.00%> (-3.34%) ⬇️
...ain/java/com/todoroo/astrid/service/TaskDeleter.kt 53.33% <0.00%> (-2.23%) ⬇️
...rc/main/java/com/todoroo/astrid/gcal/GCalHelper.kt 5.17% <0.00%> (-1.73%) ⬇️
...ava/org/tasks/notifications/NotificationManager.kt 5.55% <0.00%> (-1.67%) ⬇️
app/src/main/java/org/tasks/caldav/iCalendar.kt 54.01% <0.00%> (-0.90%) ⬇️
...ava/com/todoroo/astrid/repeats/RepeatTaskHelper.kt 75.22% <0.00%> (-0.89%) ⬇️
...pp/src/main/java/org/tasks/ui/TaskEditViewModel.kt 31.98% <0.00%> (-0.46%) ⬇️
.../main/java/com/todoroo/astrid/service/TaskMover.kt 46.42% <0.00%> (ø)
...ain/java/com/todoroo/astrid/utility/TitleParser.kt 81.46% <0.00%> (+0.69%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2220e5...20862d0. Read the comment docs.

@abaker
Copy link
Member

abaker commented Mar 1, 2021

Thanks, I merged these in 4574a5a. I added mirrors for a couple more movement related ones too (ic_airport_shuttle_24px and ic_outline_directions_walk_24px)

What are your thoughts on these two:

  • ic_outline_show_chart_24px: The x-axis on a line chart often reflects the passage of time, so should this be mirrored?
  • ic_undo_24px: The undo/redo section in the guidelines were unclear to me, they only showed examples for LTR. Maybe that means its the same in RTL

@triallax
Copy link
Contributor Author

triallax commented Mar 2, 2021

For the chart icon, I'm not really sure, but I think you can go ahead and mirror it. For the undo icon, I agree that the docs are unclear. Does the undo icon represent going back in time? Or does it represent putting something "back" to how it was? I guess you can mirror it for now, but it probably needs more examination later.

Also, quoting from above since you seemed to have missed it:

Should the left and right arrows be mirrored too? I only mirrored the back arrow.

@triallax triallax closed this Mar 2, 2021
@abaker
Copy link
Member

abaker commented Mar 2, 2021

Sorry, I wasn't sure about those. I just added mirroring back for the arrows, chart, undo, and the subdirectory back button (1f7cb2a). Hopefully someone will complain if anything is wrong

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.

2 participants