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

Designing New Splash Screen #70

Merged
merged 1 commit into from
Jun 20, 2018
Merged

Designing New Splash Screen #70

merged 1 commit into from
Jun 20, 2018

Conversation

SIMRAN1
Copy link
Contributor

@SIMRAN1 SIMRAN1 commented May 25, 2018 •
edited
Loading

Fixes #66
New Splash screen of app designed as!
image

@ghost ghost assigned SIMRAN1 May 25, 2018
@ghost ghost added the needs review label May 25, 2018
@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented May 25, 2018

@imjog @chashmeetsingh please review the pr!

@ghost
Copy link

ghost commented May 25, 2018

Hi @SIMRAN1!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

jogendra
jogendra reviewed May 26, 2018
View reviewed changes
Copy link
Member

@jogendra jogendra left a comment

Choose a reason for hiding this comment

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

Check the failing builds!

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented May 26, 2018

@imjog can you please tell me why is the build failing??

@jogendra
Copy link
Member

@SIMRAN1 Tests are failing. Run test in Xcode and see why they are failing.

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented May 26, 2018 •
edited
Loading

@imjog I have tested in xcode no test is failing!!

@ghost
Copy link

ghost commented May 26, 2018

Hi @SIMRAN1!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented May 26, 2018 •
edited
Loading

@imjog I got the test failed when I erased all contents of simulator and then run the test!
This error UIRefreshControl.init() must be used from main thread only in ScheduleViewController.swift
Shall I open issue for it?

jogendra
jogendra reviewed May 26, 2018
View reviewed changes
FOSSAsia/ScheduleViewController.swift Outdated
refreshControl.addTarget(self, action: #selector(ScheduleViewController.refreshData(_:)), for: .valueChanged)
tableView.addSubview(refreshControl)
self.refreshControl = UIRefreshControl()
refreshControl!.attributedTitle = NSAttributedString(string: "Pull to refresh")
Copy link
Member

Choose a reason for hiding this comment

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

check refreshControl with if let or guard let, to be safe!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okk @imjog but I was thinking to create another pr for resolving failed testcases !!

Copy link
Member

Choose a reason for hiding this comment

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

Do not create another PR if you resolved it here

Copy link
Member

Choose a reason for hiding this comment

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

@SIMRAN1 Please check the resolved failing tests: #73

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okk @imjog I have seen #73 !!

@jogendra jogendra mentioned this pull request May 27, 2018
Merged
@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented Jun 3, 2018

@imjog @chashmeetsingh please review the pr!!

jogendra
jogendra suggested changes Jun 3, 2018
View reviewed changes
Copy link
Member

@jogendra jogendra left a comment

Choose a reason for hiding this comment

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

  • Increase the size of the logo (refer: SUSI iOS)
  • Use the good quality of png, current png in not looking good

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented Jun 4, 2018

okk @imjog !!

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented Jun 8, 2018 •
edited
Loading

@imjog @chashmeetsingh please review and merge the pr!!

@SIMRAN1 SIMRAN1 force-pushed the issue66 branch 3 times, most recently from a123137 to 1a2f8dd Compare June 8, 2018 08:15
@ghost
Copy link

ghost commented Jun 8, 2018

Hi @SIMRAN1!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@ghost
Copy link

ghost commented Jun 8, 2018

Hi @SIMRAN1!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

jogendra
jogendra suggested changes Jun 11, 2018
View reviewed changes
Copy link
Member

@jogendra jogendra left a comment

Choose a reason for hiding this comment

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

Please remove the status bar on splash screen. Follow: https://stackoverflow.com/questions/35280747/hide-statusbar-during-splash-screen

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented Jun 11, 2018

okk @imjog !!

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented Jun 12, 2018

Done @imjog !!
image

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented Jun 12, 2018

@imjog @chashmeetsingh please review it!!

jogendra
jogendra suggested changes Jun 13, 2018
View reviewed changes
Copy link
Member

@jogendra jogendra left a comment

Choose a reason for hiding this comment

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

Bar is fine but the logo on splash screen still look small to me and logo png quality is not good. Please ask on channel if they can provide you better quality logo. Current, quality doesn't look good to me.

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented Jun 13, 2018

Thanks @imjog I will ask in channel for good quality png !!

jogendra
jogendra reviewed Jun 17, 2018
View reviewed changes
Copy link
Member

@jogendra jogendra left a comment

Choose a reason for hiding this comment

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

@SIMRAN1 Please finish one PR and then start working on other PR. This PR is made 20+ days ago and still not fulfill the requested changes. I think the issue you are solving in this PR is not a big issue.

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented Jun 18, 2018

image
@imjog I was busy with other PRs so I was not able to modify it previously ...I have done the changes please Review it!

@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented Jun 18, 2018

@imjog @jurvis @chashmeetsingh please review the pr!

jogendra
jogendra approved these changes Jun 18, 2018
View reviewed changes
Copy link
Member

@jogendra jogendra left a comment

Choose a reason for hiding this comment

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

Looking good now!

@jogendra jogendra requested a review from chashmeetsingh June 18, 2018 10:55
@SIMRAN1
Copy link
Contributor Author

SIMRAN1 commented Jun 18, 2018

thanks @imjog !

@chashmeetsingh chashmeetsingh merged commit 18f130b into fossasia:development Jun 20, 2018
@ghost ghost added ready-to-ship and removed needs review labels Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jogendra jogendra jogendra approved these changes

@chashmeetsingh chashmeetsingh Awaiting requested review from chashmeetsingh

Assignees

@SIMRAN1 SIMRAN1

Labels
ready-to-ship
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SIMRAN1 @jogendra @chashmeetsingh