Page MenuHomePhabricator
Differential D16533

Add a toggle, sticky pref, for Conpherence Widget Pane
ClosedPublic
Actions

Authored by chad on Sep 9 2016, 8:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 9:45 AM
Unknown Object (File)
Sun, Dec 8, 4:27 AM
Unknown Object (File)
Fri, Dec 6, 10:49 AM
Unknown Object (File)
Thu, Dec 5, 11:17 PM
Unknown Object (File)
Fri, Nov 29, 1:23 AM
Unknown Object (File)
Thu, Nov 28, 10:25 AM
Unknown Object (File)
Wed, Nov 20, 5:35 PM
Unknown Object (File)
Mon, Nov 18, 7:36 PM
View All Files
Subscribers
avivey
Korvin

Details

Reviewers
epriestley
Maniphest Tasks
T10364: Conpherence v4 (UI/UX)
Commits
rPff64c4e02b20: Add a toggle, sticky pref, for Conpherence Widget Pane
Summary

This adds a "column" icon into crumbs, like in workboards, for expanding or hiding the "Widget Pane". This is per user sticky and defaults to off.

Test Plan

View a Conpherence Room, see no widgets by default. Toggle it on, see widget. Reload page, see widget stick. Verify mobile, tablets ignore hiding.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

chad updated this revision to Diff 39784.Sep 9 2016, 8:04 PM
chad retitled this revision from to Add a toggle, sticky pref, for Conpherence Widget Pane.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.
Herald added a subscriber: Korvin. · View Herald TranscriptSep 9 2016, 8:04 PM
chad added a comment.Sep 9 2016, 8:05 PM

jx-viewport isn't super happy here either.

chad updated this revision to Diff 39785.Sep 9 2016, 8:45 PM
  • javascript?
chad added a comment.Sep 9 2016, 8:48 PM

I don't know how to javascript, since this doesn't work, but I think the ideas are right?

chad retitled this revision from Add a toggle, sticky pref, for Conpherence Widget Pane to [Draft] Add a toggle, sticky pref, for Conpherence Widget Pane.Sep 9 2016, 9:27 PM
chad updated this revision to Diff 39787.Sep 10 2016, 12:51 AM
  • better JS?
chad updated this revision to Diff 39788.Sep 10 2016, 1:53 AM
  • everything works
chad added a task: T10364: Conpherence v4 (UI/UX).Sep 10 2016, 2:47 AM
chad retitled this revision from [Draft] Add a toggle, sticky pref, for Conpherence Widget Pane to Add a toggle, sticky pref, for Conpherence Widget Pane.Sep 10 2016, 3:57 AM
chad updated this object.
epriestley requested changes to this revision.Sep 12 2016, 10:53 PM
epriestley edited edge metadata.

Defaulting to off seems a little weird to me, but we can give it a shot.

Slight simplification/bugfix inline but this is basically fine.

webroot/rsrc/js/application/conpherence/behavior-toggle-widget.js
15–18

Instead of doing this classname test, just do:

config.show = !config.show;
JX.DOM.alterClass(node, 'hide-widgets', config.show);
21

As written, this does the wrong thing if you click twice (or any other even number of times), since we never change config.show. That is:

  • Show column.
  • Reload page: column visible.
  • Click button twice: column hides, then visible again.
  • Reload page.
  • Expect: column visible.
  • Actual: column hidden, because this button always saves "the state opposite the state that we had when the page was loaded" when clicked.
This revision now requires changes to proceed.Sep 12 2016, 10:53 PM
epriestley added a comment.Sep 12 2016, 10:54 PM

(Fixing the classname test should also fix the other bug.)

chad updated this revision to Diff 39821.Sep 12 2016, 10:57 PM
chad edited edge metadata.
  • fix layout bugs, hide options on mobile
chad added inline comments.Sep 12 2016, 11:02 PM
webroot/rsrc/js/application/conpherence/behavior-toggle-widget.js
15–18

having trouble with it taking two clicks to toggle the first load. I think thats why I did the check, to read it right off the node.

epriestley added inline comments.Sep 12 2016, 11:06 PM
webroot/rsrc/js/application/conpherence/behavior-toggle-widget.js
21

Does this need to be flipped, since you're flipping config.show before getting here?

chad added inline comments.Sep 12 2016, 11:08 PM
webroot/rsrc/js/application/conpherence/behavior-toggle-widget.js
21

If I flip it, still takes two clicks to show or hide the column, however we send two setting requests so then it seems broken.

epriestley added a comment.Sep 12 2016, 11:08 PM

Show me the not-working code?

chad updated this revision to Diff 39822.Sep 12 2016, 11:10 PM
chad edited edge metadata.
  • update per comments
epriestley added inline comments.Sep 12 2016, 11:12 PM
webroot/rsrc/js/application/conpherence/behavior-toggle-widget.js
17

Oh, sorry -- this one needs to be flipped. Seems to work correctly to me with this change to add an !:

- JX.DOM.alterClass(node, 'hide-widgets', config.show);
+ JX.DOM.alterClass(node, 'hide-widgets', !config.show);
chad added a comment.Sep 12 2016, 11:15 PM

seems to be a lag issue for me. if I wait 5 seconds, works fine, otherwise seems to take two clicks

chad updated this revision to Diff 39823.Sep 12 2016, 11:16 PM
  • invert
epriestley accepted this revision.Sep 12 2016, 11:18 PM
epriestley edited edge metadata.

I can't reproduce any issues with it locally.

This revision is now accepted and ready to land.Sep 12 2016, 11:18 PM
Closed by commit rPff64c4e02b20: Add a toggle, sticky pref, for Conpherence Widget Pane (authored by chad, committed by chad). · Explain WhySep 12 2016, 11:20 PM
This revision was automatically updated to reflect the committed changes.
chad added a comment.Sep 12 2016, 11:20 PM

Try it live

avivey added a subscriber: avivey.Sep 13 2016, 12:58 AM

pasted_file (997×1 px, 180 KB)

The side-panel is sticky, but the main pane isn't resizing when ticking it on or off...

(If starting off and then enabling it, it hides some of the text).

On the plus side, the native scrollbar is back!

(Chrome 52/Windows)

chad added a comment.Sep 13 2016, 1:00 AM

Not sure how to reproduce this.

avivey added a comment.EditedSep 13 2016, 1:01 AM

I just toggled the thing on, reloaded the page, and switched it off again.

chad added a comment.Sep 13 2016, 1:06 AM

I can fire up windows later tonight and see. Can't reproduce on Mac. It's just adding and removing a css class.

chad added a comment.Sep 13 2016, 5:31 PM

I saw this one time in Safari/Mac, but it went away after a reload and I can't reproduce it any more. I feel like this is bad cache, but no idea why with celerity. Can't repro Chrome/FF/Safari Mac or Windows.

avivey added a comment.Sep 13 2016, 5:50 PM

Still happening to me, even after a "clear cache and hard reload".
I can also see it on MSIE when logged out - turning the side-panel on hides some of the text:

pasted_file (221×133 px, 15 KB)

Revision Contents
Changeset List

PathSize
resources/
celerity/
19 lines
src/
2 lines
applications/
conpherence/
controller/
21 lines
6 lines
view/
18 lines
settings/
setting/
12 lines
webroot/
rsrc/
css/
application/
conpherence/
11 lines
15 lines
js/
application/
conpherence/
28 lines

Diff 39824

View Options

resources/celerity/map.php

Loading...
View Options

src/__phutil_library_map__.php

Loading...
View Options

src/applications/conpherence/controller/ConpherenceController.php

Loading...
View Options

src/applications/conpherence/controller/ConpherenceViewController.php

Loading...
View Options

src/applications/conpherence/view/ConpherenceLayoutView.php

Loading...
View Options

src/applications/settings/setting/PhabricatorConpherenceWidgetVisibleSetting.php

Loading...
View Options

webroot/rsrc/css/application/conpherence/message-pane.css

Loading...
View Options

webroot/rsrc/css/application/conpherence/widget-pane.css

Loading...
View Options

webroot/rsrc/js/application/conpherence/behavior-toggle-widget.js

Loading...
Phabricator · Built by Phacility