Skip to content
This repository has been archived by the owner on May 5, 2020. It is now read-only.

Modifying AboutView Controller #393

Closed
wants to merge 5 commits into from
Closed

Modifying AboutView Controller #393

wants to merge 5 commits into from

Conversation

anubhavpulkit
Copy link
Contributor

Description

I think about view is more beautiful and impactful if it looks normal without any complexity, So I send this PR to present the same.

Fixes #243

Type of Change:

Delete irrelevant options.

  • Code
  • Quality Assurance
  • User Interface
  • Outreach

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

On simulator

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • Any dependent changes have been merged

Code/Quality Assurance Only

  • My changes generate no new warnings
  • Any dependent changes have been published in downstream modules

@sunjunkie
Copy link
Contributor

Looks nice. However, don't put that large amount of text into a storyboard element. Any future changes will cause the main.storyboard to be updated which is always ugly. More importantly, it will be impossible to localize. Use something like the Strings.swift file and add the text there. Then load the text when the view is loaded.

@anubhavpulkit
Copy link
Contributor Author

Ok @sunjunkie

@anubhavpulkit
Copy link
Contributor Author

@sunjunkie I just update this PR in the commit 4b14009
please have a look.

@sunjunkie
Copy link
Contributor

  • Indenting a paragraph is not necessary when there is a blank line between paragraphs. Use the blank line not the indentation.
  • Change the 2nd heading to "Why is Powerup needed?"
  • Have you looked at Attributed Strings in iOS. You can easily highlight the headings using that.
  • What is the following code doing in AboutViewController.swift?
        aboutViewText.safeAreaInsets.left
        aboutViewText.safeAreaInsets.right
        aboutViewText.safeAreaInsets.bottom

@anubhavpulkit
Copy link
Contributor Author

  • Indenting a paragraph is not necessary when there is a blank line between paragraphs. Use the blank line not the indentation.
  • Change the 2nd heading to "Why is Powerup needed?"
  • Have you looked at Attributed Strings in iOS. You can easily highlight the headings using that.
  • What is the following code doing in AboutViewController.swift?
        aboutViewText.safeAreaInsets.left
        aboutViewText.safeAreaInsets.right
        aboutViewText.safeAreaInsets.bottom

ok, ooh this is my mistake

        aboutViewText.safeAreaInsets.left
        aboutViewText.safeAreaInsets.right
        aboutViewText.safeAreaInsets.bottom

I will add corrections as early as possible.

@anubhavpulkit
Copy link
Contributor Author

@sunjunkie I don't know why it fails Ci test but on my system, it works fine without any error.

@sunjunkie
Copy link
Contributor

The 2nd heading needs to be changed as instructed.
The text and Title ("About") is not lined up on the left side.
Your approach to setting the about text in the view is not a good one. When I click the about button, there is a screen flicker before the actual text is shown. Look at UIScrollView and Attributed Strings.
Do not use "magic numbers" in your code. Use constants instead. Constants make code clearer and less error prone.

@anubhavpulkit
Copy link
Contributor Author

@sunjunkie I can't understand the last line what is "magic number" and I think I use constraints on every single element in About VIew.
The text and Title ("About") is not lined up on the left side. if the main title "About" and sub-titles and text aligned then it does not look good I tried it before giving indentation to "About".
The main reason for this indentation is to show About is the main title.

and screen flicker before the actual text is shown because we use a function to print that text if we use attribute inspector to print the text then it works smoothly without any flicker.

@anubhavpulkit
Copy link
Contributor Author

anubhavpulkit commented Apr 18, 2020

@sunjunkie Can I use attribute inspector to print the text? and what about the position of the main title "About" can I change position for lining up with sub-title and text?

@anubhavpulkit
Copy link
Contributor Author

anubhavpulkit commented Apr 18, 2020

Looks nice. However, don't put that large amount of text into a storyboard element. Any future changes will cause the main.storyboard to be updated which is always ugly. More importantly, it will be impossible to localize. Use something like the Strings.swift file and add the text there. Then load the text when the view is loaded.

I think the first commit is best to merge.
I think the last person who works on about view they put whole text in the storyboard element because of the same reason of screen flicker before the actual text is shown.

@sunjunkie
Copy link
Contributor

Magic Number: https://en.wikipedia.org/wiki/Magic_number_(programming)

The current PR is not a good enough solution.

@sunjunkie sunjunkie closed this Apr 18, 2020
@anubhavpulkit
Copy link
Contributor Author

Ok, @sunjunkie Im starts again(with new energy) and fix this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modifying AboutView Controller
2 participants