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

Changes in search control bar #259

Closed
wants to merge 2 commits into from

Conversation

sashrikakaur
Copy link

@sashrikakaur sashrikakaur commented Mar 29, 2019

Signed-off-by: Sashrika Kaur [email protected]

Fixes #252

The cancel search button was overlapping with the sky map toolbar. Adding a margin prevents the overlap and allows easy access of the menu in skymap toolbar and search button in the search control bar.

ezgif com-video-to-gif

@tavishjain
Copy link

tavishjain commented Mar 30, 2019

This is a really bad implementation of how you have fixed the code. Simply giving a padding to the app bar doesn't completely fix this issue

@jaydeetay
Copy link
Member

Thanks for the fix @sashrikakaur . It looks like there's a conflict in gradle.xml - can you remove that file from the PR? We have a separate task add this file to .gitignore.

@tavishjain can you explain your objections a little more? At first glance this looks like a reasonable fix.

@tavishjain
Copy link

@jaydeetay this shifting of the app bar is not the exact fix for this issue. We will have to use CoordinatorLayout so that on press on the screen, the app bar moves down and on tapping again, it moves back up

@jaydeetay
Copy link
Member

jaydeetay commented Mar 30, 2019 via email

@tavishjain
Copy link

@jaydeetay it is not bad that the search controls remain in the same place, but it provides a bad UX. Having a free space abbove the app bar (the one in Red colour) doesnt reallly seem to be good.
Untitled

@sashrikakaur
Copy link
Author

Considering sky map uses OpenGL ES to render views adding a padding doesn't necessarily mean a bad UI as the whole view is used in the app to display celestial objects and so does the space above the screen.
Hence, I think this would be a good fix as the space present is not empty it still renders a view.
Let me know if you have an alternate solution @jaydeetay @tavishjain , I'll be happy to hear your implementation for the same!

@sashrikakaur
Copy link
Author

@jaydeetay , I've resolved the conflict, let me know if everything looks good now.

@sashrikakaur
Copy link
Author

closed in favor of #264

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

Successfully merging this pull request may close these issues.

3 participants