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

Updated About view Controller #397

Closed
wants to merge 6 commits into from
Closed

Updated About view Controller #397

wants to merge 6 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

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

about view.mov.zip

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • Any dependent changes have been merged

Code/Quality Assurance Only

  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

@anubhavpulkit
Copy link
Contributor Author

All additional changes are complete in this PR.

  • 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.
  • The text and Title ("About") is not lined up on the left side.
  • When I click the about button, there is a screen flicker before the actual text is shown.

@anubhavpulkit
Copy link
Contributor Author

@sunjunkie Please have a look this PR.

@sunjunkie
Copy link
Contributor

This is better.

  • You are not using attributed strings so please remove that item from the checklist.
  • The right margin for the text should be moved to the left more - so it is more in line with the current version.
  • Why did you put the question strings in the code? Move them into the string constants file.

@sunjunkie
Copy link
Contributor

(I accidentally put this comment in the wrong PR! 😟)

This looks great. But you reverted the 2nd question - "Why is Powerup needed?" :-)

So while you're at it: you repeat the same UIColor specifications 6 times. It should be in a constant so any changes must done only once. Repeating code like that creates a chance for introducing a bug when making changes. Consider the following instead:

      let aboutColor = UIColor(red: 74/255, green: 160/255, blue: 166/255, alpha: 1.0)

       let attributesHeading: [NSAttributedString.Key : Any] = [
           .foregroundColor: aboutColor,
          .font: UIFont(name: "HelveticaNeue-Bold", size: 24)!
       ]
       let attributesText: [NSAttributedString.Key : Any] = [
          .foregroundColor: aboutColor,
          .font: UIFont(name: "HelveticaNeue", size: 20)!
       ]

       attributedString.addAttributes(attributesHeading, range: NSRange(location: 0, length: 8))
       attributedString.addAttributes(attributesText, range: NSRange(location: 10, length: 578))
       
      ...

@sunjunkie sunjunkie closed this Apr 22, 2020
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