Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Add TiDialogFragment #20

Closed

Conversation

remcomokveld
Copy link
Contributor

Speaks for itself. Although I can think of prettier, delegate based, solutions to add MVP support for DialogFragments this is a simple copy from TiFragment but it extends android.support.v4.app.DialogFragment instead of android.support.v4.app.Fragment

@passsy
Copy link
Contributor

passsy commented Oct 6, 2016

please have a look at this @lacky1991

@StefMa
Copy link
Contributor

StefMa commented Oct 11, 2016

Don't know if i like the idea.
Have you really so much business logic inside your Dialog? 🤔
However. If we do it, then we need to use a Delegate and merge all the duplicated code from TiFragment and TiDialogFragment...

@passsy
Copy link
Contributor

passsy commented Oct 11, 2016

Yes, a delegate is definitely required. This is also required when we want to support TiFragment and TiDialogFragment for the plugin.
As a first step I'm ok with the duplicated code. But I remember @lacky1991 found a edge case when he implemented TiDialogFragment himself.

@remcomokveld
Copy link
Contributor Author

If I have a dialog where I can edit a data object and what to save/sync this changes, I need to have a tested presenter which does that business logic. Totally agree with the fact that it should be implemented with a delegate

@StefMa
Copy link
Contributor

StefMa commented Nov 4, 2016

@remcomokveld are you still working on it?
It seems that more people needs the DialogFragment than I thought 😃

@passsy passsy added this to the 0.8.0 milestone Nov 4, 2016
@Nxt3
Copy link

Nxt3 commented Nov 4, 2016

I noticed that while using this the layout of my Dialog gets wonky. If I extend AppCompatDialogFragment instead of TiDialogFragment--the layout is fine. Not sure what the cause could be.

Here is the layout for my dialog

Correct (extending AppCompatDialogFragment)
screenshot_20161104-174540

Incorrect (extending TiDialogFragment)
screenshot_20161104-174124

@passsy
Copy link
Contributor

passsy commented Nov 5, 2016

Looks fine for me with the TiDialogFragment extends DialogFragment.
screenshot-2016-11-05_15 37 00 507

And I can't imagine the code of TiDialogFragment causes this. My best guess is some caching bug of instant run 😉

@Nxt3
Copy link

Nxt3 commented Nov 5, 2016

I don't use instant run. It seems to be the difference of extending "DialogFragment" vs "AppCompatDialogFragment"

@passsy
Copy link
Contributor

passsy commented Nov 5, 2016

Now I got it! Yes the TiDialogFragment should extend AppCompatDialogFragment and not DialogFragment.

I think this here causes the difference:

    // AppCompatDialogFragment
    @Override
    public Dialog onCreateDialog(Bundle savedInstanceState) {
        return new AppCompatDialog(getContext(), getTheme());
    }
    // DialogFragment
    public Dialog onCreateDialog(Bundle savedInstanceState) {
        return new Dialog(getActivity(), getTheme());
    }

AppCompatDialogFragment also hints:

A special version of {@link DialogFragment} which uses an {@link AppCompatDialog} in place of a platform-styled dialog.

import java.util.List;

public abstract class TiDialogFragment<P extends TiPresenter<V>, V extends TiView>
extends DialogFragment implements TiPresenterProvider<P>, TiLoggingTagProvider,
Copy link
Contributor

Choose a reason for hiding this comment

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

should extend AppCompatDialogFragment instead of DialogFragment

@StefMa
Copy link
Contributor

StefMa commented Nov 5, 2016

@StefMa StefMa mentioned this pull request Dec 13, 2016
@StefMa
Copy link
Contributor

StefMa commented Dec 13, 2016

It seems @remcomokveld don't maintain this anymore.
I've just created another PR with the AppCompat fix. See #42.

This will be closed.

@StefMa StefMa closed this Dec 13, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants