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

Allow popup height to be changed with a setter #373

Merged
merged 9 commits into from
Sep 24, 2019

Conversation

VitalyKuznetsov
Copy link
Contributor

Adding functionality to EmojiPopup to be opened in a different mode:
Keyboard height is not dynamically calculated, but set manually instead.

Keyboard height is not dynamically calculated, but set manually instead.
@mario
Copy link
Collaborator

mario commented Jul 30, 2019

I'd prefer the approach that was mentioned earlier - allow setting height in the builder and if it's 0, calculate the height - otherwise use set height.

@VitalyKuznetsov
Copy link
Contributor Author

Consider the following case:

  1. User opens app and uses emojiPopup -> height is set in builder.
  2. User changes keyboard height in settings.
  3. Goes back to the app, emojiPopup has not been destroyed yet.
  4. Upon keyboard opening we save its height, however emojiPopup height cannot be changed, until a new instance is created.

For such scenario a simple setter would be quite useful.

@VitalyKuznetsov
Copy link
Contributor Author

So what do you think?

It will use its current height, and calculate it from the first keyboard opening only.
…t mode: Keyboard height is not dynamically calculated, but set manually instead.
@VitalyKuznetsov
Copy link
Contributor Author

Hey, @mario , take a look!
Maybe it is more like what you are looking for?

Copy link
Owner

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

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

You should not create multiple EmojiPopup's hence when you initialize it you should know the height.

Why do you need the set method on the Popup itself?

emoji/src/main/java/com/vanniktech/emoji/EmojiPopup.java Outdated Show resolved Hide resolved
Co-Authored-By: Niklas Baudy <niklas.baudy@vanniktech.de>
@VitalyKuznetsov
Copy link
Contributor Author

Yeah sure, but the gboard has an option to change its height from settings, which may occure when emojipopup is already created.
In this case I'd like to change its height without creating a new instance.

vanniktech
vanniktech previously approved these changes Aug 14, 2019
Copy link
Owner

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

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

Alright. I'm cool with it. Let's wait for @mario

@mario
Copy link
Collaborator

mario commented Aug 18, 2019

I'll test this in the coming days.

@VitalyKuznetsov
Copy link
Contributor Author

Any updates? @mario

@mario
Copy link
Collaborator

mario commented Sep 13, 2019

Looks ok.

@vanniktech
Copy link
Owner

@VitalyKuznetsov can you fix the CI?

@vanniktech vanniktech changed the title Popup-height changed with a setter. Allow popup height to be changed with a setter Sep 23, 2019
vanniktech
vanniktech previously approved these changes Sep 23, 2019
@mario
Copy link
Collaborator

mario commented Sep 24, 2019

Can be merged after rebase.

Copy link
Owner

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

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

Thank you!

@vanniktech vanniktech merged commit e7964ec into vanniktech:master Sep 24, 2019
@vanniktech
Copy link
Owner

@VitalyKuznetsov are you still using this method and if so; can you show me your code which detects that the keyboard height has been changed and hence you are manually calling this method?

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