Bugfix/fix thread composer ux #2936

bishwenduk029 · 2018-04-21T18:44:32Z

Status

Ready for Review
hyperion (frontend) I am not sure on this, but I did made changes so I think this may be needed

I created this pull request to be sure that I am coding the fix correctly, so if someone could please review and give any feedback, would be great. Thanks You.

closes #2872

dev-drprasad · 2018-04-22T06:12:38Z

@bishwenduk029 you have to put x inside [] like [x] while creating pull request so that it will be checkbox. You can add closes #2872 at the end of pull request template so that PR will tagged automatically in relevant issue

bishwenduk029 · 2018-04-22T06:17:37Z

Oh sorry my bad... @dev-drprasad thanks a ton man.I have updated the same accordingly.

dev-drprasad

@bishwenduk029 There is lot of redundant logic in handleIncomingProps method. since we dont need to know active community, we can remove that logic.

dev-drprasad · 2018-04-22T07:24:40Z

src/components/composer/index.js

+                onChange={this.setActiveChannel}
+                value={activeChannel}
+              >
+                <option key={-1} value="">


As for I know we don't need key prop here. we use key prop only when we do .map. though i might be wrong

@dev-drprasad Oh I think you were reviewing an older version actually, I made some updates to the PR afterwards to remove redundant code. As for the key parameter, I think post render the above option also becomes a part of the rendered array of options(via . map). Since all are now siblings so key should be unique I think.

brianlovin · 2018-04-22T21:20:58Z

One feedback on the design - if we are moving the controls next to the publish button we don't need the 'to' bar in the top. And the order of the actions in the bottom bar should be:

[cancel]                   [community dropdown][channel dropdown][publish]

If that makes sense :)

dev-drprasad · 2018-04-23T04:23:17Z

@bishwenduk029 good to put first menu item in dropdowns like Select community/Select Channel. what do you say ? @brianlovin

littleStudent · 2018-04-23T05:32:44Z

src/components/composer/index.js

      availableChannels: channels,
-      activeCommunity: community ? community.id : null,
-      activeChannel: activeChannel ? activeChannel.id : null,
+      activeCommunity: '',


so this means that i have to select a community every time?
If i have already selected a single community at the left side navigation we should also pre-select this community here

Agreed - it should be pre filtered if the user has selected a community or channel in the left sidebar.

Sure I am on it to make the necessary changes for pre-selected community/channel.

brianlovin · 2018-04-23T19:07:13Z

@dev-drprasad yup, sounds good 😀

littleStudent · 2018-04-25T05:11:34Z

src/components/composer/index.js

+    const showSelectedCommunity = activeCommunity
+      ? activeCommunity.length > 0
+      : false;
+    const showSelectedChannel = activeChannel


showSelectedChannel and showSelectedCommunity are unused atm. Dont know if you plan on using them? :)

No, I need to remove them, thanks @littleStudent .

mxstbr

Thanks for tackling this! I tested it locally, but I don't seem to be able to select a community or channel, the <select> is empty for me?

Also, now that we don't have a header anymore we should get rid of that huge amounts of space at the top.

spectrum-bot · 2018-04-29T12:31:39Z

	Warnings
⚠️	These modified files do not have Flow enabled: src/components/composer/style.js src/components/threadComposer/components/composer.js

Generated by 🚫 dangerJS

bishwenduk029 · 2018-04-29T13:09:15Z

@mxstbr thanks. I checked the use case and it seems I missed an edge case when none of the communities are selected, sorry my bad. So I have made the changes accordingly.

mxstbr

Getting there—it is now defaulting to the first community again, but it should default to no community, and it doesn't unlock the "Publish" button when you have both a community and a channel selected.

…shwenduk029/spectrum into bugfix/fix-thread-composer-ux

bishwenduk029 · 2018-04-30T20:12:30Z

@mxstbr , I have made the changes accordingly to allow a user to select community in the event there are no pre-selected communities. But for unlocking the Publish button, I need some help understanding. Because I thought title of the thread is also another necessary input for unlocking the Publish button.

1.) Community Selected &
2.) Channel Selected &
3.) Title provided

Unlock the Publish button. Please correct me if I am wrong, I will make the change accordingly.

mxstbr · 2018-05-01T15:45:42Z

Oh yeah that is correct, my bad! 😅 I'll take another look at this tomorrow if @brianlovin doesn't beat me to it!

mxstbr

This works super great now, exactly how I imagined! 😍

@brianlovin want to give this some last polish and then we ship this?

bishwenduk029 · 2018-05-02T14:58:34Z

Wow gr8!! Thanks @mxstbr and thanks everyone @dev-drprasad @brianlovin and @littleStudent for helping out and also being patient with me. It is really an awesome community to work in. Do you think similar changes would be needed in mobile counterpart of the app.

brianlovin · 2018-05-03T17:26:58Z

Looks like there's a failing test here on composing new threads - @bishwenduk029 would you be able to look into this? TEST_DB=true yarn run dev:api and yarn run cypress:open and then the cypress/integration/thread_spec.js test looks to be the culprit :)

brianlovin · 2018-05-03T17:30:00Z

It also looks like this is fairly broken on mobile - the thread content has some overflow issues, the composer doesn't take up the full height of the view, and there doesn't seem to be a way to leave the composer on mobile.

bishwenduk029 · 2018-06-10T08:10:23Z

@brianlovin tried fixing the mobile screen discrepancies, can you please review and let me know your feedback on the same. Sorry for the large screenshot below tried reducing the size but could not

Regards

bishwenduk029 · 2018-07-16T02:56:05Z

@mxstbr, @brianlovin , @littleStudent and @dev-drprasad , can you please review PR once and let me know if any further changes are needed

mxstbr · 2018-08-06T12:17:03Z

Sorry for the super slow reply here @bishwenduk029, I'm taking a look now!

mxstbr · 2018-08-06T12:19:15Z

This is actually looking great, not sure why we haven't shipped this yet! Thanks so much @bishwenduk029 🙏

I've got one tiny nit that I'm going to look at before merging this:

On the community and channel view, it shows the "Cancel" button even though it shouldn't:

mxstbr · 2018-08-06T12:25:57Z

Actually, this also has some pretty big issues on mobile:

Not sure how to best resolve this, any ideas @brianlovin?

brianlovin · 2018-08-07T03:28:06Z

It looks like this branch just needs some work on mobile to flow the content and actions properly. I haven't had much time to put thought into this design though, sorry.

brianlovin · 2019-02-22T04:42:31Z

We've made a lot of improvements to this with #4575, so going to close this as it's a bit stale. Always room for mobile ux improvements though :)

Bishwendu Kundu added 2 commits April 21, 2018 17:45

Initial Changes for the fix

2026edb

Changes completed to publish only when community, channel and texts

ba3e9a6

spectrum-bot bot added the WIP: Building label Apr 22, 2018

some cosmetic changes

9404c16

dev-drprasad reviewed Apr 22, 2018

View reviewed changes

littleStudent suggested changes Apr 23, 2018

View reviewed changes

Bishwendu Kundu added 2 commits April 25, 2018 00:09

nomral changes

8aa7316

Incorported the review comments

91e9a97

littleStudent reviewed Apr 25, 2018

View reviewed changes

Bishwendu Kundu and others added 2 commits April 25, 2018 10:53

remove redundant code

a06c7cb

Fix flow

f163335

mxstbr reviewed Apr 26, 2018

View reviewed changes

Bishwendu Kundu and others added 9 commits April 29, 2018 16:26

Initial Changes for the fix

993297a

Changes completed to publish only when community, channel and texts

71d54ab

some cosmetic changes

99abd18

nomral changes

c949378

Incorported the review comments

2258620

remove redundant code

e87d32f

Fix flow

cdfbd41

Select a community randomly, if not selected by the user

da233fd

merge conflicts

4fa5924

merge conflicts resolved

0fef5c6

spectrum-bot bot added the Ready for Review label Apr 29, 2018

Bishwendu Kundu and others added 5 commits April 30, 2018 08:19

nomral changes

3500e91

Incorported the review comments

d01a95d

remove redundant code

5907f6e

Fix flow

91a77a0

merge conflicts resolved

f6dfd03

mxstbr suggested changes Apr 30, 2018

View reviewed changes

Bishwendu Kundu added 2 commits May 1, 2018 01:30

Show communities when none selected by user

0403d84

Merge branch 'bugfix/fix-thread-composer-ux' of https://github.com/bi…

154385d

…shwenduk029/spectrum into bugfix/fix-thread-composer-ux

Fix Flow errors

514bfd7

mxstbr previously approved these changes May 2, 2018

View reviewed changes

littleStudent previously approved these changes May 2, 2018

View reviewed changes

Fix cypress test cases

4e8b25e

bishwenduk029 dismissed mxstbr’s stale review via 4e8b25e May 4, 2018 02:53

Fix mobile screens by switching between Flex row & column

26e28a2

bishwenduk029 dismissed littleStudent’s stale review via 26e28a2 June 10, 2018 07:33

Merge branch 'alpha' into bugfix/fix-thread-composer-ux

7a51f5e

Merge branch 'alpha' into bugfix/fix-thread-composer-ux

77b5282

brianlovin closed this Feb 22, 2019

Bugfix/fix thread composer ux #2936

Bugfix/fix thread composer ux #2936

Uh oh!

Conversation

bishwenduk029 commented Apr 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dev-drprasad commented Apr 22, 2018

Uh oh!

bishwenduk029 commented Apr 22, 2018

Uh oh!

dev-drprasad left a comment

Choose a reason for hiding this comment

Uh oh!

dev-drprasad Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

bishwenduk029 Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

brianlovin commented Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dev-drprasad commented Apr 23, 2018

Uh oh!

littleStudent Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

brianlovin Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

bishwenduk029 Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

brianlovin commented Apr 23, 2018

Uh oh!

littleStudent Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

bishwenduk029 Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

mxstbr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spectrum-bot bot commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bishwenduk029 commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mxstbr left a comment

Choose a reason for hiding this comment

Uh oh!

bishwenduk029 commented Apr 30, 2018

Uh oh!

mxstbr commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mxstbr left a comment

Choose a reason for hiding this comment

Uh oh!

bishwenduk029 commented May 2, 2018

Uh oh!

brianlovin commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brianlovin commented May 3, 2018

Uh oh!

bishwenduk029 commented Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bishwenduk029 commented Jul 16, 2018

Uh oh!

mxstbr commented Aug 6, 2018

Uh oh!

mxstbr commented Aug 6, 2018

Uh oh!

mxstbr commented Aug 6, 2018

bishwenduk029 commented Apr 21, 2018 •

edited

Loading

brianlovin commented Apr 22, 2018 •

edited

Loading

mxstbr left a comment •

edited

Loading

spectrum-bot bot commented Apr 29, 2018 •

edited

Loading

bishwenduk029 commented Apr 29, 2018 •

edited

Loading

mxstbr commented May 1, 2018 •

edited

Loading

brianlovin commented May 3, 2018 •

edited

Loading

bishwenduk029 commented Jun 10, 2018 •

edited

Loading