Skip to content
This repository has been archived by the owner on Dec 19, 2022. It is now read-only.

Bug 1046485 - Move search bar out of SearchFragment #43

Merged
merged 1 commit into from Aug 5, 2014
Merged

Bug 1046485 - Move search bar out of SearchFragment #43

merged 1 commit into from Aug 5, 2014

Conversation

leibovic
Copy link
Collaborator

No description provided.

this.searchState = searchState;

preSearch.setVisibility(searchState == SearchState.PRESEARCH ? View.VISIBLE : View.INVISIBLE);
postSearch.setVisibility(searchState == SearchState.POSTSEARCH ? View.VISIBLE : View.INVISIBLE);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a ViewSwitcher could be helpful in this class. It would remove a few lines of code and a reference, but it would also make the intent very clear: only one of these views will be visible at a given time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll file a follow-up bug to look into this. It seems like ViewSwitchers are mainly used for animating between two different views, but it could be useful to be explicit about only ever showing one of these views. To reduce potential regressions, I think it's best to just keep the layout the same for this refactor.

@EricEdens
Copy link
Contributor

I left some small feedback items, but overall I really like the direction of this refactoring.

  • Fragments communicate through MainActivity (they don't reach into each other)
  • There are clear APIs for communication (onSuggest, loadSuggestions, etc)
  • Visibility states are managed through a single place (MainActivity)

Looks good :)

leibovic added a commit that referenced this pull request Aug 5, 2014
Bug 1046485 - Move search bar out of SearchFragment. r=eedens
@leibovic leibovic merged commit 58d6a4b into mozilla:master Aug 5, 2014
@leibovic leibovic deleted the refactor branch August 5, 2014 23:17
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.

None yet

2 participants