-
Notifications
You must be signed in to change notification settings - Fork 555
fix: Text overlapping in search fragment #651
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
Conversation
|
@iamareebjamal Please review it. |
| private fun checkAndLoadFragment(fragment: Fragment) { | ||
| val savedFragment = supportFragmentManager.findFragmentByTag(fragment::class.java.name) | ||
| if (savedFragment != null) { | ||
| if (savedFragment != null && fragment !is SearchFragment) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a fix. This is a workaround
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a fix. This is a workaround
But as in Eventbright everytime open search section, it opens with a new fragment so, I think no saved state is required for search fragment. That's why I direct load with new fragment if the search fragment is open. Should I fix this issue in another way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the special case should be handled in a generic way while opening search fragment, not a special case in a method which should not even know about the existence of search fragment
|
@iamareebjamal Updated. |
|
Are you still working on it ? @liveHarshit I will be happy to solve this issue |
Yes. |
|
@liveHarshit please finish this! |
|
Updated. Now I check the saved state for search fragment. If yes, then hide the search layout for no overlapping with saved data. |
|
No changes in Activity are needed. Only changes should be in the fragment as it is the buggy component. Use fragment lifecycles or ViewModel to fix the bug |
|
Updated. |
| private lateinit var rootView: View | ||
| private var loadEventsAgain = false | ||
| private lateinit var searchView: SearchView | ||
| private var isSearched = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this variable from here, react to changes from viewmodel only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| }) | ||
|
|
||
| searchViewModel.isSearched.observe(this, Observer { | ||
| showSearchLayout(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always sending false, it should send the value received from the LiveData instead
| searchViewModel.searchEvent = query | ||
| rootView.searchLinearLayout.visibility = View.GONE | ||
| rootView.fabSearch.visibility = View.GONE | ||
| showSearchLayout(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No explicit calls from view, all calls should be from ViewModel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
|
I request you to look at the first solution you submitted and this one and realize which one was elegant and maintainable. 😄 Good work |
Fixes #644
Changes: Set search fragment without the saved state.