Feature/sharing update to fluxc #5933

nbradbury · 2017-05-25T00:20:30Z

Updates the long-awaited sharing feature to support FluxC.

…d of blog

…er instead that the WebViewFragment passes in

…fluxc

…ment

…-mobile/WordPress-Android into feature/sharing-update-to-fluxc

…ction" This reverts commit fe92de5.

kwonye

Looks great!

One question: why are you passing in a SiteModel to PublicizeWebView/Detail/ListFragment instead of the siteId? Looks like siteId is all we use there still. Not blocking, just curious.

kwonye · 2017-05-25T20:50:44Z

WordPress/src/main/java/org/wordpress/android/ui/publicize/PublicizeWebViewFragment.java

@@ -157,9 +162,6 @@ private void loadConnectUrl() {
        public void onPageFinished(WebView view, String url) {
            super.onPageFinished(view, url);

-            // TODO: remove logging from final - only here for debugging
-            AppLog.d(AppLog.T.SHARING, "onPageFinished > " + url);


Great catch

kwonye · 2017-05-25T20:54:26Z

WordPress/src/main/res/layout/publicize_list_fragment.xml

@@ -51,7 +51,7 @@
                android:textColor="@color/grey_dark"
                android:textSize="@dimen/text_sz_medium" />

-            <org.wordpress.android.widgets.WPButton
+            <Button


This leaves the button barely visible. I'm ok with it since this still has to go through the design work. Just want to make sure you know.

I'm not sure how that would affect the button's size, but that was a change you made here: 4590c8f

Sorry, didn't mean size, I meant background. Yea, I made the change, just wanted to make sure it was conscious that it wasn't touched 👍

kwonye · 2017-05-25T20:59:43Z

...ess/src/main/java/org/wordpress/android/ui/publicize/PublicizeManageConnectionsFragment.java

@@ -191,7 +182,7 @@ private void configureSharingButtons(JSONObject response) throws JSONException {
    protected void initPreferences() {
        configureSharingAndMoreButtonsPreferences();
        mLabelPreference = (SummaryEditTextPreference) findPreference(getString(R.string.publicize_label));
-        mButtonStylePreference = (DetailListPreference) findPreference(getString((R.string.publicize_button_style));
+        mButtonStylePreference = (DetailListPreference) findPreference(getString(R.string.publicize_button_style));


Thanks for this clean up 👍

nbradbury · 2017-05-25T21:34:42Z

why are you passing in a SiteModel to PublicizeWebView/Detail/ListFragment instead of the siteId?

I did that because it's the convention used elsewhere in the app, but you're right - we really only need to pass the site ID. Changed in 0ff514e.

kwonye · 2017-05-25T21:45:59Z

I did that because it's the convention used elsewhere in the app

Oh, if that's the case I'm totally down with keeping it! I'm still unfamiliar with FluxC and would rather stay with convention than optimize a tiny model passing through.

kwonye · 2017-05-25T22:22:29Z

oguzkocer · 2017-05-25T22:59:17Z

why are you passing in a SiteModel to PublicizeWebView/Detail/ListFragment instead of the siteId?

I know this is already merged, but the idea behind passing the model instead of the id is to avoid confusion about if that's a local id or a remote one. So, if we do want to pass the id, we should name the variables to reflect their type. So, in this case using ARG_SITE_REMOTE_ID instead of ARG_SITE_ID would probably better. I hope this helps!

For reference, here are @maxme's comments on the subject: #5856 (comment) & #5856 (comment)

maxme · 2017-05-26T05:09:47Z

ut the idea behind passing the model instead of the id is to avoid confusion about if that's a local id or a remote one

Yes exactly this. It's not a big deal but we should only see ids when we call the REST api (we shouldn't see then if we use FluxC actions) and keep the model everywhere else.

kwonye · 2017-05-26T15:30:48Z

@oguzkocer @maxme thanks for the info! @nbradbury can you revert 0ff514e in your next PR please? Sorry about that.

kwonye and others added 30 commits May 3, 2017 14:18

Fix merge issue that overwrote viewBlogSettings function

1008e50

Calling a new method for viewing Sharing screen with SiteModel instea…

f363510

…d of blog

Retrieving siteId from SiteModel passed in

343a833

Matching to develop branch

bc87343

Removing fetching of account in PublicizeActions and adding a paramet…

bb83af6

…er instead that the WebViewFragment passes in

Replacing use of AccountHelper with AccountStore

439a184

Replacing WPButton with Button

4590c8f

Replacing use of AccountHelper with AccountStore

ee9e77b

Converting the use of SettingsFragment to AccountSettingsFragment

ed37a4f

Using the accountStore to get info

627eac5

Showing to .com sites only

e3baa03

Declaring parameter final

2c5521a

Updating to use left

8d8c063

Merge branch 'feature/sharing-master' into feature/sharing-update-to-…

a6b0c2b

…fluxc

Updating gradle plugin

78f98f5

Merge branch 'feature/sharing-master' into feature/sharing-update-to-…

766c166

…fluxc

Pass site to PublicizeManageConnectionsFragment

0107fcf

Remove mSiteId from PublicizeListActivity

8593aca

Retrieve site settings

bda1fe7

Use findPreference rather than getChangePref

014ec3b

PublicizeManageConnectionsFragment now inherits from SiteSettingsFrag…

80a921e

…ment

PublicizeManageConnectionsFragment now compiles

413a285

Pass current user ID to PublicizeServiceAdapter

449c84d

Pass current user ID to PublicizeConnectionAdapter

f75a342

Merge branch 'feature/sharing-master' of https://github.com/wordpress…

6f7ed2b

…-mobile/WordPress-Android into feature/sharing-update-to-fluxc

Inject AccountStore into PublicizeWebViewFragment

d3992db

Inject AccountStore into PublicizeDetailFragment

79ee076

Inject AccountStore into PublicizeListFragment

a6572fb

Pass SiteModel to PublicizeListFragment

b0d6df0

Pass SiteModel to PublicizeWebViewFragment

b9ef8c2

nbradbury added 10 commits May 24, 2017 17:43

Pass SiteModel to PublicizeDetailFragment

c2a5c25

Use long siteId

f7e2ce2

Use correct long siteId for PublicizeTable and PublicizeConnection

fe92de5

Revert "Use correct long siteId for PublicizeTable and PublicizeConne…

5fd7499

…ction" This reverts commit fe92de5.

Use correct long siteId for PublicizeTable and PublicizeConnection

8aa7524

Pass the long siteId to PublicizeServiceAdapter

c0b20f3

Pass the long siteId to PublicizeUpdateService

e1c774b

Removed siteId param from showListFragment

d1ff838

Pass the long siteId to PublicizeConnectionAdapter

016130c

Re-enable WebViewUtils.clearCookiesAsync()

872e4c2

nbradbury added the Sharing label May 25, 2017

nbradbury assigned kwonye May 25, 2017

kwonye approved these changes May 25, 2017

View reviewed changes

Pass only the siteId

0ff514e

kwonye merged commit ef8c7ed into feature/sharing-master May 25, 2017

kwonye deleted the feature/sharing-update-to-fluxc branch May 25, 2017 22:35

nbradbury mentioned this pull request May 26, 2017

feature/sharing-use-site-model-not-id #5954

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/sharing update to fluxc #5933

Feature/sharing update to fluxc #5933

nbradbury commented May 25, 2017

kwonye left a comment

kwonye May 25, 2017

kwonye May 25, 2017

nbradbury May 25, 2017

kwonye May 25, 2017

kwonye May 25, 2017

nbradbury commented May 25, 2017

kwonye commented May 25, 2017

kwonye commented May 25, 2017

oguzkocer commented May 25, 2017

maxme commented May 26, 2017 •

edited

Loading

kwonye commented May 26, 2017

Feature/sharing update to fluxc #5933

Feature/sharing update to fluxc #5933

Conversation

nbradbury commented May 25, 2017

kwonye left a comment

Choose a reason for hiding this comment

kwonye May 25, 2017

Choose a reason for hiding this comment

kwonye May 25, 2017

Choose a reason for hiding this comment

nbradbury May 25, 2017

Choose a reason for hiding this comment

kwonye May 25, 2017

Choose a reason for hiding this comment

kwonye May 25, 2017

Choose a reason for hiding this comment

nbradbury commented May 25, 2017

kwonye commented May 25, 2017

kwonye commented May 25, 2017

oguzkocer commented May 25, 2017

maxme commented May 26, 2017 • edited Loading

kwonye commented May 26, 2017

maxme commented May 26, 2017 •

edited

Loading