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

Adding a static menu and necessary components #324

Merged
merged 17 commits into from
Jun 11, 2023

Conversation

SaurabhJamadagni
Copy link
Collaborator

Contributor checklist

  • This pull request is on a separate branch and not the main branch
  • I have updated the CHANGELOG with a description of my changes for the upcoming release (if necessary)

Description

Hey @andrewtavis! Like I mentioned in our meeting last week, sorry for this clutter of a pull request. I promise the next pull requests will be atomic and specific in nature. I was extremely rusty with UIKit and needed a lot of refresher articles which really clumped the commits together beyond resolve.

  • This pull request adds a Tab bar navigation to the Scribe app. We can toggle between Installation, Settings, About sections.
  • The Installation tab is just the older Scribe app screen. Will make changes to it as I get to their respective child issues.
  • The Settings and About tab use a nested UITableView approach. They have their separate files to interpolate the tables on these screens.
    • A few of the older commits will basically show the older approach I was attempting where each section was a separate UITableView.
    • It required having multiple IBOutlet variables and also needed a ScrollView to be added manually. (Adding constraints to the ScrollView was a mess)
    • Nested table views allow us to have only a single table view and also helps with as the content scales or screen size reduces.
    • The custom cell components can be found in the Components folder.
  • Necessary icons have been added to the Assets catalogue along with their dark mode versions.

Tested on devices iPhone 14 Pro, iPhone SE (3rd gen)

Related issue

@andrewtavis andrewtavis self-requested a review June 5, 2023 15:39
@andrewtavis
Copy link
Member

Thanks @SaurabhJamadagni! I'll give this a look in the coming days and let you know if we're good to go 😊 GSoC! ☀️

Section(sectionTitle: "See the code on GitHub", imageString: "github", hasToggle: false),
Section(sectionTitle: "Chat with the team on Matrix", imageString: "matrix", hasToggle: false),
Section(sectionTitle: "Wikimedia and Scribe", imageString: "wikimedia", hasToggle: false),
Section(sectionTitle: "Share Scribe", imageString: "square.and.arrow.up", hasToggle: false)
Copy link
Member

Choose a reason for hiding this comment

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

Hey hey 👋

Would texts like these, the sectionTitles here, be good candidates for i18n? I see some support for it started already under ./Scribe/AppTexts. Thinking that the app texts here could be good ways as well to continue working i18n into Scribe.


Implementing via the below perhaps?

@andrewtavis
Copy link
Member

Yes, definitely :) We can maybe make a separate issue for i18n to be implemented sooner rather than later so we can keep up on it as we go?

@andrewtavis
Copy link
Member

andrewtavis commented Jun 5, 2023

@SaurabhJamadagni, as discussed in the weekly:

  • Row elements should have an increased height
  • There should be more padding between the icon and text for table elements
  • Page headers should be inline with the tables (table headers should be indented a little bit as they are already)

@SaurabhJamadagni
Copy link
Collaborator Author

SaurabhJamadagni commented Jun 6, 2023

  • Row elements should have an increased height
  • There should be more padding between the icon and text for table elements
  • Page headers should be inline with the tables (table headers should be indented a little bit as they are already)

Hey @andrewtavis, currently working on the unchecked todo above. A small hiccup with dynamic resizing the table rows. The parent table cells aren't responding to the row height change. Will push an update soon.

@SaurabhJamadagni
Copy link
Collaborator Author

Commit a25c0bf should do it. Let me know @andrewtavis if everything is in order. Thanks!

@SaurabhJamadagni
Copy link
Collaborator Author

Hey @andrewtavis, just a small git related question. If I push my branch changes to remote, they'll be appended in this pull request itself right? So I should I push the changes to the same request or wait so that issue #315 has it's own separate pull request after this one is merged?

@andrewtavis
Copy link
Member

You’d be welcome to push to this PR, @SaurabhJamadagni :) Sorry for not reviewing yet. Lots to do for the new place this week and I wanted to clear the PR backlog for activist before jumping over here.

Whatever is easier for you, but don’t sit with changes waiting for me :)

@andrewtavis
Copy link
Member

I have one more activist PR I want to get to and will then jump over here. Hopefully tonight, but if not then tomorrow!

@SaurabhJamadagni
Copy link
Collaborator Author

No worries @andrewtavis, I'll wait for this PR to be merged. In case any new contributors wish to see how a certain issue was resolved or implemented they might have to come to this giant combined PR otherwise. Might get overwhelming 😅

@andrewtavis
Copy link
Member

Sounds good, @SaurabhJamadagni! As I said I’ll try to get to it tonight :)

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

Minor comments on this, @SaurabhJamadagni (and of course sorry for getting to this late - GF's dad has been in town and I've been helping them out):

  • Feel free to remove the View privacy policy button, but then I'm assuming your plan is to do that in Privacy policy menu tab in about section #317 :)
  • I'll go through and do some minor code formatting for spaces between }s
  • Structure is really solid and I really appreciate the thought that you put into it!
  • I'll doubtless play around with it a bit in the coming weeks and make some minor minor changes as I go, but I'm really happy with this 😊 Crazy to see Scribe like this on my computer, and even crazier to think of what's to come and actually having this on devices! Thank you! 🎉

@andrewtavis andrewtavis merged commit ba736f1 into scribe-org:main Jun 11, 2023
@andrewtavis
Copy link
Member

andrewtavis commented Jun 11, 2023

@SaurabhJamadagni
Copy link
Collaborator Author

Hey @andrewtavis! No problem with the delay, it was nothing :)

Just got back yesterday evening from the hackathon I told you last week. Thank you so much for the review. Completely with you on the progress on Scribe. It is super exciting to be able to contribute to it!! Thanks for taking care of any formatting that I might have missed. See you tonight for our check-in 😊

@andrewtavis
Copy link
Member

Looking forward to hearing about the hackathon later!

@andrewtavis andrewtavis added the GSoC Available for Google Summer of Code participants label Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GSoC Available for Google Summer of Code participants
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants