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.

Sign up for GitHub

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

Jump to bottom

Table view headers and footers in Settings. #5011

Merged
merged 8 commits into from
Oct 21, 2021
Merged

Table view headers and footers in Settings. #5011

merged 8 commits into from
Oct 21, 2021

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Oct 14, 2021 •
edited
Loading

Builds upon the older #3406. This draft version is using the system default font sizes and doesn't have support the Discovery and User Settings links that are currently in the app.

@niquewoodhouse: Two questions:

  • Looking at the Settings figma, there doesn't appear to be the need for the existing Discovery and User Settings links in the future. Do you think they are OK to leave out now?
  • What do you think about using the system's default font sizes for these? The header size matches the figma above, but the footer is slightly smaller.

Frame 1

Pull Request Checklist

  • I read the contributing guide
  • UI change has been tested on both light and dark themes, in portrait and landscape orientations and on iPhone and iPad simulators
  • Accessibility has been taken into account.
  • Pull request is based on the develop branch
  • Pull request contains a changelog file in ./changelog.d
  • You've made a self review of your PR
  • Pull request includes screenshots or videos of UI changes
  • Pull request includes a sign off

@pixlwave pixlwave self-assigned this Oct 14, 2021
@pixlwave pixlwave added A-Settings Z-Papercuts Visible. Impactful. Predictable to action. labels Oct 14, 2021
@pixlwave pixlwave mentioned this pull request Oct 14, 2021
Closed
5 tasks
@github-actions
Copy link

github-actions bot commented Oct 14, 2021 •
edited
Loading

📱 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/fUkzNE

@niquewoodhouse
Copy link

  • Looking at the Settings figma, there doesn't appear to be the need for the existing Discovery and User Settings links in the future. Do you think they are OK to leave out now?

I would suggest leaving that as it is today and not removing it. I don't know how final that Figma is, personally it sounds great but the papercuts project is about making changes that aren't controversial/need to be undone. So I'd suggest leaving that change to a separate project, but it may become a papercut in time itself.

  • What do you think about using the system's default font sizes for these? The header size matches the figma above, but the footer is slightly smaller.

The footer you're using is slightly smaller than the Figma? Are you using footnote as the size? I think that's fine but subhead might work slightly better if possible.

@pixlwave
Copy link
Member Author

Super thanks for taking a look.

I would suggest leaving that as it is today and not removing it.

👍

The footer you're using is slightly smaller than the Figma? Are you using footnote as the size? I think that's fine but subhead might work slightly better if possible.

Yes, it's 13pt so footnote. I'll give subhead a go.

@pixlwave
Copy link
Member Author

pixlwave commented Oct 15, 2021 •
edited
Loading

@niquewoodhouse One more question. In the following 2 screenshots, do you think the information cell in the Discovery section should be footers as well?

I'm inclined to say yes (maybe using the alert colour for the error message), but wanted to check first.

artboard

Decided actions after a discussion:

  • Use the footer style for the cases above
  • Update the Accept button text to Accept Identity Server Terms
  • Remove Invite Friends setting as it's not a setting and is also found directly above the Settings button in the sidebar.
  • Merge the first 2 cells from the Other section into the Advanced and rename this to Information (or other name to follow).

@pixlwave pixlwave marked this pull request as ready for review October 20, 2021 13:41
@pixlwave
Copy link
Member Author

Now this is ready for review, final screenshots in both light and dark mode are below. The fonts in the SwiftUI screens were updated to match.

final

pixlwave and others added 4 commits October 20, 2021 17:40
@pixlwave
Use headerTextPrimaryColor in headers and don't override system font …
8e8efd4 
…size.
@pixlwave
Use footnote text style for description cells to match UITableView's …
80e6e4a 
…footer style. Match the footer colour to the header colour. Add -[SettingsViewController descriptionCellForTableView:] and use for appropriate cells.
@pixlwave
Use actual footer titles rather than cells that mimic them.
685d8f2
@pixlwave
Update fonts, add attributed strings and add tap gestures back.
4b5630a 
Footers use subheadline. Headers use footnote.
@pixlwave
Use footers for app and discovery information.
7a096a7 
Remove Invite Friends button from settings.
Reorganise Advanced and Other settings.
Rename Other to About.
@pixlwave
Fix bug accepting identity server terms from the Setting Discovery se…
2f879e8 
…ction.
@pixlwave
Add SectionFooterView.xib to fix fonts on iOS 12-14.
614cb4d 
Update SecurityViewController too.
@pixlwave
Address Steve's comments.
a7395db
@pixlwave
Copy link
Member Author

Sorry I needed to rebase, so I've had to force push 😕. But all the changes are in that last commit.

@pixlwave pixlwave merged commit 30b0a83 into develop Oct 21, 2021
@pixlwave pixlwave deleted the doug/headers_and_footers branch October 21, 2021 13:52
@pixlwave pixlwave removed their assignment Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SBiOSoftWhare SBiOSoftWhare SBiOSoftWhare approved these changes

@niquewoodhouse niquewoodhouse niquewoodhouse approved these changes

Assignees
No one assigned
Labels
A-Settings Z-Papercuts Visible. Impactful. Predictable to action.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pixlwave @niquewoodhouse @SBiOSoftWhare