[#330] Add description texts to cells #384

damien-rivet · 2023-11-20T08:52:14Z

Description

This PR adds the description labels to the settings cells.

Under the hood, it does far more things than that, here's an exhaustive list:

Add a localisation catalog (only English is filled for now)
Add a BaseTableViewController to share setup logic with all TableViewVCs
Add a dedicated AboutTableViewCell
Refactor AboutTabViewController to inherit BaseTableViewController
Refactor TableViewTemplateViewController to inherit BaseTableViewController
Remove image view from info tableview cell
Remove ParentTableViewCell (used native section instead and changed TableViewVCs to use grouped inset)
Move interaction logic from info tableview cell to respective screens

Design of the cells might evolve again depending on the needs that may arise.

Screenshots

	iPhone	iPad
Settings
Language settings
About

Related issue

Add menu option description functionality and description texts #330

github-actions · 2023-11-20T08:52:38Z

Thank you for the pull request!

The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and iOS rooms once you're in. It'd be great to have you!

Maintainer checklist

The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution
- The contributor's name and icon in remote commits should be the same as what appears in the PR
- If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local Scribe-iOS repo
The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

damien-rivet · 2023-11-20T13:01:03Z

I might even be able to shave off some files by using the native UITableViewCell instead of custom cells now that I got rid of the intermediate UITableView (when I removed the ParentTableViewCell) 🤔

damien-rivet · 2023-11-20T13:03:59Z

Scribe/Views/BaseTableViewController.swift

+      headerView = UIView(frame: CGRect(x: 0, y: 0, width: tableView.bounds.width, height: sectionHeaderHeight))
+    }
+
+    let label = UILabel(frame: CGRect(x: 0, y: 0, width: headerView.bounds.width, height: sectionHeaderHeight))


Note for myself, check that I'm not adding a UILabel on top of another UILabel each time the header is requested.

Scribe/SettingsTab/SettingsViewController.swift

Scribe/Components/InfoChildTableViewCell/InfoChildTableViewCell.swift

andrewtavis · 2023-11-30T01:48:30Z

Very sorry for the wait on all this, @damien-rivet.

I might even be able to shave off some files by using the native UITableViewCell instead of custom cells now that I got rid of the intermediate UITableView (when I removed the ParentTableViewCell) 🤔

If you're able to simplify things down further this sounds reasonable to me :)

Comments on the overall design of the changes, which are expansive and include some things that you doubtless are planning on getting to:

The divider lines would ideally not span the whole view and would have an equal-width gap from the view's right side
Would you say that we shouldn't do chevrons on the About page options?
- I was thinking that having them for everything makes sense for consistency as we have Privacy policy and Third-party licenses that go to a sub page in a similar way that elements of Settings do (French and Italian above)
- As those aforementioned options on About would have the chevron, then including it for all the elements for visible consistency made sense to me
A bit of a shadow on the table views would be good
- Hex for the shadow in the designs is #3F3F46
I wrote out an ask for an increase in corner rounding and then tried what you implemented in the designs and I think your version looks much cleaner 😊
- Designs have been updated :)
I also updated the designs to partially reflect what you have for Settings in so far as I don't think we need descriptions for App language, Increase app text size or High color contrast
- Is it possible to do App settings as the section header and App language with a chevron and default for the field?
- We can also do a different issue to finalize this 😊
- Sets it up for some issues for color contrast and text size later

andrewtavis · 2023-11-30T01:55:24Z

Scribe/AboutTab/AboutViewController.swift

@@ -1,42 +1,47 @@
 //
-//  AboutViewController.swift
+//  Copyright (C) 2023 Scribe


Would you suggest putting this copyright notice at the head of each file?

Since the project is based on the GNU license, it is advised by the license itself to add this header on top of each code file.

I'm adding it to new files and files I'm refactoring only, doing a complete pass would add nothing and generate extra commit noise, it can be done over time instead.

I'll make an issue for this, @damien-rivet! Thanks for bringing this to my attention :)

See #391 😊

andrewtavis · 2023-11-30T01:59:14Z

Scribe/AboutTab/AboutViewController.swift

-
-    let nib = UINib(nibName: "ParentTableViewCell", bundle: nil)
-    outerTable.register(nib, forCellReuseIdentifier: "ParentTableViewCell")
+    title = NSLocalizedString("about.title", comment: "The title of the about tab")


Really beautiful stuff, and great that you jumped into already using the naming conventions we're planning on 🙌

Thanks 😉 A good thing with this system is that the naming can be changed and it will update the catalog with the new keys (not sure it will move the values accordingly though 🤔 ).

andrewtavis · 2023-11-30T02:05:34Z

Scribe/AboutTab/AboutViewController.swift

+
+      present(mailComposeViewController, animated: true, completion: nil)
+    } else {
+      /// Show alert mentioning the email address


Note for nit for period that I can add :)

andrewtavis · 2023-11-30T02:13:04Z

Scribe/Resources/Localizable.xcstrings

@@ -0,0 +1,157 @@
+{


Really appreciate the focus on L10n :) We might end up switching this over to a JSON as the general idea is that we'll be linking it to the Android repo as well (see Scribe-i18n#1), but for now this is great and sets us up perfectly for the later switch!

Hmm good point 🤔 At least the call sites can be found pretty easily 😄

Made #392 to cover this :)

andrewtavis · 2023-11-30T02:21:46Z

First sweep is done and I've also gone through and tested the menu options to make sure that everything is being reflected in the keyboard after options are selected. All's working really well, and the code is dramatically easier to follow given the new changes 😊 Let me know how much of the comments above you'd like to implement, @damien-rivet! I'm happy to look those that you feel are out of scope for the current task :)

Changed static data to constants to avoid unwanted modifications. Added localization catalog to the Scribe App. Added a shortDescription field to cell model.

Add a base table view controller to implement similar logic between screens.

Additional clean up.

damien-rivet · 2023-12-15T08:18:51Z

Very sorry for the wait on all this, @damien-rivet.

I might even be able to shave off some files by using the native UITableViewCell instead of custom cells now that I got rid of the intermediate UITableView (when I removed the ParentTableViewCell) 🤔

If you're able to simplify things down further this sounds reasonable to me :)

Comments on the overall design of the changes, which are expansive and include some things that you doubtless are planning on getting to:

The divider lines would ideally not span the whole view and would have an equal-width gap from the view's right side

I'll look into this, it should be a matter of tweaking the separator insets 🤔

Would you say that we shouldn't do chevrons on the About page options?

I was thinking that having them for everything makes sense for consistency as we have Privacy policy and Third-party licenses that go to a sub page in a similar way that elements of Settings do (French and Italian above)

As those aforementioned options on About would have the chevron, then including it for all the elements for visible consistency made sense to me

You're absolutely right, chevron needs to be used whenever there's a nested navigation that comes from the user tapping a cell, so it makes sense to add them whenever it is needed.

A bit of a shadow on the table views would be good

Hex for the shadow in the designs is #3F3F46

I'll check if it possible without changing drastically the code, the tableview itself cover the whole screen and the cells group is not shadowable that easily. Apple's own HIG suggested at some point that subtle shadows should be used but now there's no mention of it at all 🤔

I wrote out an ask for an increase in corner rounding and then tried what you implemented in the designs and I think your version looks much cleaner 😊

Designs have been updated :)

I've used a basic corner radius, it can be tweaked if we needed to change it 😉

I also updated the designs to partially reflect what you have for Settings in so far as I don't think we need descriptions for App language, Increase app text size or High color contrast

Is it possible to do App settings as the section header and App language with a chevron and default for the field?

We can also do a different issue to finalize this 😊

Nah I'll do it by updating the table data for the settings screen.

Sets it up for some issues for color contrast and text size later

Added separator insets to tableviews. Added disclosure indicators in About tabs to cells with nested navigation. Additional cleanups.

damien-rivet · 2023-12-16T20:53:16Z

@andrewtavis I've fixed most of the comments, there's only the one related to adding back the table's shadow that's still left to do.

Once this PR is merged, I'll create an issue for that.

andrewtavis · 2023-12-20T19:07:11Z

Thank you, @damien-rivet! Really appreciate your continued efforts here :) I'll get to the review by EOW as it's a bit of a busy one for me :)

damien-rivet · 2023-12-28T22:11:19Z

@andrewtavis don't sweat it 😉 There's no rush, end of the year is hard on everyone as there's plenty to handle both personally and professionally.

I hope you had a great Christmas (or equivalent) and I hope you'll have a wonderful New Year's eve ^^

andrewtavis · 2023-12-29T17:40:48Z

Thank you for the understanding and the message, @damien-rivet 😊 Really does help with some of the nerves of not being able to context switch over to this given all that's going on.

The holidays were relaxed in a good way, and Chaos Communication Congress has been a lot of fun so far! Hoping that you and yours have had a nice Christmas (or equivalent) as well, and wishing you all the best for the end of the year and start of the next!

andrewtavis · 2024-01-13T12:34:29Z

Scribe/AboutTab/AboutTableData.swift

      ],
      hasDynamicData: nil
    ),
    ParentTableCellModel(
      headingTitle: "Legal",
      section: [
-        Section(sectionTitle: "Privacy policy", imageString: "lock.shield", hasToggle: false, sectionState: .privacyPolicy),
-        Section(sectionTitle: "Third-party licenses", imageString: "thirdPartyLicenses", hasToggle: false, sectionState: .licenses),
+        Section(sectionTitle: "Privacy policy", imageString: "lock.shield", hasNestedNavigation: true, sectionState: .privacyPolicy),


Switching it to nested navigation as a check makes a lot more sense, @damien-rivet :)

andrewtavis

Very sorry for the wait here, @damien-rivet. Slowly trying to get back into more active maintenance of Scribe, and I really appreciate your patience and support for the project!

All looks good to me here. As stated in the comments, I've made two issues to cover the license requirements and to shift the i18n over to JSONs, but those should be easy enough to achieve :) The last things I'm seeing that are needed here are the shadows under the menu fields and external link icons for those options that link to the web like the Matrix and GitHub options. I'll make issues for those now!

damien-rivet force-pushed the feature/330-add-settings-options-information-tooltips branch from c6d717d to 5bd24f3 Compare November 20, 2023 09:01

damien-rivet commented Nov 20, 2023

View reviewed changes

Scribe/SettingsTab/SettingsViewController.swift Show resolved Hide resolved

damien-rivet force-pushed the feature/330-add-settings-options-information-tooltips branch from 5bd24f3 to 276affd Compare November 20, 2023 13:10

damien-rivet commented Nov 20, 2023

View reviewed changes

Scribe/Components/InfoChildTableViewCell/InfoChildTableViewCell.swift Show resolved Hide resolved

andrewtavis self-requested a review November 30, 2023 00:57

andrewtavis reviewed Nov 30, 2023

View reviewed changes

damien-rivet added 4 commits December 15, 2023 08:52

[scribe-org#330] Add information tooltips to settings options

0f38536

Changed static data to constants to avoid unwanted modifications. Added localization catalog to the Scribe App. Added a shortDescription field to cell model.

[scribe-org#330] Refactor cells and tableviews

2bed3f1

Add a base table view controller to implement similar logic between screens.

[scribe-org#330] Refactor TableViewTemplateVC to use BaseTableViewVC

26a22b8

Additional clean up.

[scribe-org#330] Add a dedicated cell for About tab

040dbd0

damien-rivet force-pushed the feature/330-add-settings-options-information-tooltips branch from 276affd to 040dbd0 Compare December 15, 2023 07:52

[scribe-org#330] Apply PR feedback

ac0979f

Added separator insets to tableviews. Added disclosure indicators in About tabs to cells with nested navigation. Additional cleanups.

damien-rivet requested a review from andrewtavis December 23, 2023 18:05

andrewtavis reviewed Jan 13, 2024

View reviewed changes

andrewtavis mentioned this pull request Jan 13, 2024

Add license information at the top of each Scribe file #391

Closed

2 tasks

andrewtavis approved these changes Jan 13, 2024

View reviewed changes

andrewtavis merged commit 5087842 into scribe-org:main Jan 13, 2024

This was referenced Jan 13, 2024

Add shadows to menu options for the Settings and About screens #394

Closed

Add menu option description functionality and description texts #330

Closed

damien-rivet deleted the feature/330-add-settings-options-information-tooltips branch March 22, 2024 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#330] Add description texts to cells #384

[#330] Add description texts to cells #384

damien-rivet commented Nov 20, 2023

github-actions bot commented Nov 20, 2023 •

edited by andrewtavis

Loading

damien-rivet commented Nov 20, 2023 •

edited

Loading

damien-rivet Nov 20, 2023

andrewtavis commented Nov 30, 2023

andrewtavis Nov 30, 2023

damien-rivet Dec 15, 2023 •

edited

Loading

andrewtavis Jan 13, 2024

andrewtavis Jan 13, 2024

andrewtavis Nov 30, 2023

damien-rivet Dec 15, 2023 •

edited

Loading

andrewtavis Nov 30, 2023

andrewtavis Nov 30, 2023

damien-rivet Dec 15, 2023

andrewtavis Jan 13, 2024

andrewtavis commented Nov 30, 2023

damien-rivet commented Dec 15, 2023

damien-rivet commented Dec 16, 2023

andrewtavis commented Dec 20, 2023

damien-rivet commented Dec 28, 2023

andrewtavis commented Dec 29, 2023

andrewtavis Jan 13, 2024

andrewtavis left a comment

[#330] Add description texts to cells #384

[#330] Add description texts to cells #384

Conversation

damien-rivet commented Nov 20, 2023

Description

Screenshots

Related issue

github-actions bot commented Nov 20, 2023 • edited by andrewtavis Loading

Thank you for the pull request!

Maintainer checklist

damien-rivet commented Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

andrewtavis commented Nov 30, 2023

Choose a reason for hiding this comment

damien-rivet Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damien-rivet Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewtavis commented Nov 30, 2023

damien-rivet commented Dec 15, 2023

damien-rivet commented Dec 16, 2023

andrewtavis commented Dec 20, 2023

damien-rivet commented Dec 28, 2023

andrewtavis commented Dec 29, 2023

Choose a reason for hiding this comment

andrewtavis left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 20, 2023 •

edited by andrewtavis

Loading

damien-rivet commented Nov 20, 2023 •

edited

Loading

damien-rivet Dec 15, 2023 •

edited

Loading

damien-rivet Dec 15, 2023 •

edited

Loading