teerox / Nimble-iOS-Challenge

0 stars 0 forks source link

UI Issues #8

Open minhnimble opened 2 years ago

minhnimble commented 2 years ago

It is interesting to see you made certain choices for the UI implementations and actually you went overboard to implement the survey screen in full details while it is not a part of this challenge requirements. Now, there are still some noticeable issues with your current implementation that I would like to list them down as follow:

teerox commented 2 years ago

I totally agree with you, on all comments, i will address them accordingly.

  1. on the keyboard covering the password field, i intend to use the IQKeyboardManager library to handle that, or using Notification to listen to keyboard being active and pulling the screen upward to the rightful position.

  2. I think i must have forgot to add this to the fields because i already had an UIView extension that adds corner radius

  3. I couldn't add right padding, my implantation refuse to add right padding to the forgot password, i had use a hack which is to give extra small space after the after the forgot password like "Forgot ". as a temporary fix😁

    1. I have an algorithm that checks for valid email somewhere but my brain must have failed me not to remember using itπŸ™ˆ and i was trying as much as possible to avoid using external libraries cos some can be unreliable.
    2. Yes i can check them independently, just trying to speed up development, to be able to cover as much task as required.
    3. Yea, i should have perform the loading on the main thread to freeze it, so that user doesn't perform any action while loading is happening
    4. Really cant explain this issue thou.
    5. For this, i didn't test but my i made sure my network module returns a message for any failed api call. i know if i tested this scenario i would have fixed it.

Survey Screen

  1. Yea, on this i should have implemented something like this @IBAction func pageControltapped(_ sender: Any) { guard let pageControl = sender as? UIPageControl else { return } let selectedPage = pageControl.currentPage } but in all trying to use the data the endpoint provided was really a huge task, cos i had to understand to the best of my knowledge and since this is a test i just had to do the best i can for now. but i know i would have done better with my implementation

  2. on the shimmer loader, my possible solution was to add corner radius to the views, the wired test showing was actually hard coded.. i made sure to remove all hard coded text.

  3. That avatar not loading is because the url that returns the user avatar that is the user profile endpoint returns a url that doesn't load and my possible fix would have been to have the design avatar as a fall back plan if the the network call for loading image fails

  4. For this, i only had to use the images returned by the url, and then set it to aspect fill, just so the image fills the screen with out losing quality.

  5. on this, yea. i could do better using the horizontal transition which i update implement from the Animation method

  6. Yea, i totally agree with you.

In Summary, I know i could do better. this was really tasking and trying to combine the task and my current job literally didn't give me a break. Atleast the new things i learnt from the task was using animations and double tasking.

minhnimble commented 2 years ago

It is great to see your quick responses πŸ€— Regarding your comments, I would like to follow up with them as below:

on the keyboard covering the password field, i intend to use the IQKeyboardManager library to handle that, or using Notification to listen to keyboard being active and pulling the screen upward to the rightful position.

IQKeyboardManager is actually a good choice for solving this problem. Can't wait to see it in action πŸ‘

I couldn't add right padding, my implantation refuse to add right padding to the forgot password, i had use a hack which is to give extra small space after the after the forgot password like "Forgot ". as a temporary fix😁

Hmm do you know if there is any better way to do this that we can still keep the trailing constraint of the button to the textfield's trailing and increase the spacing without having to add a space in the text?

Yes i can check them independently, just trying to speed up development, to be able to cover as much task as required.

Oh I understand now. As of this point I would like to know you thoughts on how you would balance the delivery and the quality of a product? Do you favor delivery more over quality or vice versa? and why?

Yea, i should have perform the loading on the main thread to freeze it, so that user doesn't perform any action while loading is happening

Interesting point, so just to confirm the technique you would use to prevent user interactions is to perform the loading on the main thread, right? but isn't the loading indicator already performed on main thread? Or do you mean we need to call the Login API on the main thread to freeze it?

For this, i didn't test but my i made sure my network module returns a message for any failed api call. i know if i tested this scenario i would have fixed it.

No worries, for this point I just want to know further your opinion on the matter, as I can see that you already has some basic error checks when calling an API already. πŸ‘

Yea, on this i should have implemented something like this @IBAction func pageControltapped(_ sender: Any) { guard let pageControl = sender as? UIPageControl else { return } let selectedPage = pageControl.currentPage } but in all trying to use the data the endpoint provided was really a huge task, cos i had to understand to the best of my knowledge and since this is a test i just had to do the best i can for now. but i know i would have done better with my implementation

Nice response, it seems you know what needs to be done. Looking forward to seeing your fix for this problem. πŸ’ͺ

That avatar not loading is because the url that returns the user avatar that is the user profile endpoint returns a url that doesn't load and my possible fix would have been to have the design avatar as a fall back plan if the the network call for loading image fails

Sounds like a valid solution to me. πŸ‘Œ

For this, i only had to use the images returned by the url, and then set it to aspect fill, just so the image fills the screen with out losing quality.

Actually, maybe you missed some information from the Postman API documents that you can get the high-res version of an image by just appending β€œl” to the image URL obtained in the API response. Can we apply that for better image quality?