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

Fix bugs when building with Xcode 13. #5422

Merged
merged 8 commits into from
Feb 3, 2022
Merged

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Jan 24, 2022

Fixes #4883.

A couple of notes on the context menu:

  • I've used SF Symbols as the outlined style is more familiar in UIKit's context menus, however some shapes are inconsistent with our other images 😕
  • It is only added on the Home tab in this PR, but Favourites / People / Rooms could be updated if desired.
  • The menu items are labeled are as follows:
Enable Disable
Direct Chat Room
Mute or Notifications** Un-mute or Notifications**
Favourite Un-favourite
Low Priority Normal Priority
Leave

** Notifications is shown by default and presents the notifications settings screen. Some forks may have this disabled and those will show Mute/Un-mute instead

Last Steps:

  • Add localisations for the strings
Tab bar (without content underneath) Context menu
Simulator Screen Shot - iPhone 13 mini - 2022-01-26 at 19 04 02 Simulator Screen Shot - iPhone 13 mini - 2022-01-26 at 19 04 17
A low priority muted room A favourited DM with notifications A low priority DM
Simulator Screen Shot - iPhone 13 mini - 2022-01-26 at 19 40 50 Simulator Screen Shot - iPhone 13 mini - 2022-01-26 at 19 40 24 Simulator Screen Shot - iPhone 13 mini - 2022-01-26 at 19 30 30

@github-actions
Copy link

github-actions bot commented Jan 24, 2022

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

QR code

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/Knqizo

@pixlwave pixlwave force-pushed the doug/4883_xcode_13_fixes branch 2 times, most recently from 2246d0d to 542b40b Compare January 26, 2022 19:53
@pixlwave
Copy link
Member Author

@daniellekirkwood / @amshakal Could you take a look at the changes to the context menu on this PR please? I've done what makes sense to me but I think the "Direct Chat/Room" string is probably no clearer than the icon was on its own and the changes to the icons clearly need reviewing.

@daniellekirkwood
Copy link
Contributor

I'm comfortable with the change to the context menu on the Home screen, and the icons (as they're similar enough to communicate the same thing but also recognisable Apple icons).

What I'd like to discuss further is the content in this menu... My suggestion would be:

Current copy Suggested copy
Direct chat/ Room Move to People / Move to Rooms
Mute/Unmute or Notifications Mute/ Unmute or Notifications
Favourite/Un-favourite Favourite / Remove from Favourites
Low priority/Normal priority Low priority / Normal priority
Leave Leave

@ShadowJonathan
Copy link
Contributor

Mute/ Unmute or Notifications

(this seems to be missing a space before the /?)

@daniellekirkwood
Copy link
Contributor

Mute/ Unmute or Notifications

(this seems to be missing a space before the /?)

The copy would either be Mute or Unmute dependant on the current setting the user has

@pixlwave
Copy link
Member Author

@daniellekirkwood In terms of string length, screenshots below with the longest string on the smallest phone.

iPhone SE iPhone SE w/ larger font
Simulator Screen Shot - iPod touch (7th generation) - 2022-01-27 at 16 03 15 Simulator Screen Shot - iPod touch (7th generation) - 2022-01-27 at 16 03 42

@pixlwave
Copy link
Member Author

One last comment on the symbols, the one for leave isn't supported on iOS 13, so it is currently using xmark instead.

Simulator Screen Shot - iPhone 11 - 2022-01-27 at 16 38 42

Copy link
Contributor

@daniellekirkwood daniellekirkwood left a comment

Choose a reason for hiding this comment

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

Looks good to me. Will need @amshakal 's review on Monday

Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

Would like to make the context menu preview provider a bit more generic, otherwise looks good to me 👍

Riot/Modules/Home/HomeViewController.m Outdated Show resolved Hide resolved
Riot/Modules/Home/RoomContextPreviewViewController.swift Outdated Show resolved Hide resolved
Riot/Modules/Home/HomeViewController.m Outdated Show resolved Hide resolved
Riot/Modules/Home/HomeViewController.m Show resolved Hide resolved
Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

👏

@amshakal
Copy link

amshakal commented Feb 3, 2022

Hello, just a few comments:

  1. Agree with all copy changes suggested above besides 'remove from favourites'. Can we stick to un-favourite? I think the long string length makes it less scalable.
  2. I think x mark is fine for now, I am sure we'll be able to update the icon later on. Do we have options that we can choose from?

Thank you!

@daniellekirkwood
Copy link
Contributor

  1. Agree with all copy changes suggested above besides 'remove from favourites'. Can we stick to un-favourite? I think the long string length makes it less scalable.

Not sure how easy "un-favourite" is to translate so we might have to show 'un-favourite' in english and provide the "remove from favourites" string for other regions to translate?

I'd rather us go with "Remove from Favourites" but happy to roll with @amshakal's suggestion.

@pixlwave
Copy link
Member Author

pixlwave commented Feb 3, 2022

Do we have options that we can choose from?

These are all from the SF Symbols library. If you don't have it, you can install the symbol browser from here: https://developer.apple.com/sf-symbols/

You can check what will work on iOS 13 by looking for an availability of 1.0 in sidebar.

@amshakal
Copy link

amshakal commented Feb 3, 2022

  • Okay, that's complicated. We can stick to 'Remove from Favourites', just really wish it could be shortened.

  • As for symbols, seems like sticking with x makes sense for now.

@pixlwave pixlwave merged commit 3b6dcc3 into develop Feb 3, 2022
@pixlwave pixlwave deleted the doug/4883_xcode_13_fixes branch February 3, 2022 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants