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

[V2] Hide menu when messages return null #2754

Merged
merged 1 commit into from
Jun 29, 2018

Conversation

JedWatson
Copy link
Owner

Alternative solution to #2719

@JedWatson JedWatson changed the base branch from master to v2 June 27, 2018 07:59
@JedWatson JedWatson merged commit d955def into v2 Jun 29, 2018
@JedWatson JedWatson deleted the v2-hide-menu-with-null-message branch June 29, 2018 05:10
gwyneplaine pushed a commit that referenced this pull request Jun 29, 2018
@gwyneplaine
Copy link
Collaborator

@JedWatson @jossmac leaving the thoughts we discussed circa this PR here.
We have agreed that this is the most pragmatic way to resolve this issue for the time being, however conflating the implicit responsibility of noOptionsMessage and loadOptionsMessage props with the larger concern of rendering the menu seems not ideal for the following reasons:

  • By tying together two unrelated concerns like this to fulfill a niche use case, we may be setting ourselves up for more perilous api entanglement further down the road. Any additional augmentations to menu behaviour (or the ability to customise menu behaviour) will now need to factor this relationship into consideration.
  • We've built a more explicit event handling api and configuration interface to allow users to replicate these sorts of niche relationships, with the information we provide them, a solution that allows for this would be (alongside the benefits of the first point mentioned) contributing to a more consistent api.
  • We may be blocking other niche cases and customization points here that we have no yet encountered. For example users that would like to not show a results message but still show header and footer elements they've made available in the menu.
  • We now have menu rendering behaviour that sits outside of the core menuIsOpen prop, that we expose and document as the core value for controlling menu rendering behaviour.

**Note the above are reasons are not enough to dissuade us from implementing this solution for now, as it is an easy solution to revert and resolves the use case in a backwards compatible manner, but will be a useful reference should we ever decide to come back this PR.

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.

None yet

2 participants