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

Hide section #275

Merged
merged 29 commits into from
Dec 13, 2017
Merged

Hide section #275

merged 29 commits into from
Dec 13, 2017

Conversation

tokejepsen
Copy link
Member

This PR is an attempt at being able to hide sections of items in the UI. This is just the basic working version, so everything is up for discussion.

dec 8 2017 12_23 pm

Its implemented similar to how the toggling works, by having a item.isHidden property in the model. I'm thinking that this will allow us to implement a way to customize which sections are shown on launch, which could address #189

@tokejepsen
Copy link
Member Author

I see I have #273 in here as well. Maybe we could merge that first, instead of having to reopen a PR.

@BigRoy
Copy link
Member

BigRoy commented Dec 8, 2017

The behavior to click the header and toggle all instances underneath it? Is that still doable? If so, how does one do that now? You seem to do it in the GIF but I'm not sure how.

@BigRoy
Copy link
Member

BigRoy commented Dec 8, 2017

Also, I think it's looking good. But with the headers being able to collapse I must say when they are collapsed it's confusing if one of the stages gave errors, because you wouldn't see it. As such the headers must likely become much more informative to in a way allow showing such data? Or would it always show warned/errored plug-ins?

@tokejepsen
Copy link
Member Author

You seem to do it in the GIF but I'm not sure how.

Yup, you can still do that. The clickable area is the labels boundary. Just the first solution I could think off, so suggestions for better alternatives are welcome.

Or would it always show warned/errored plug-ins?

Yeah, I was thinking we could colour the label depending on the succcess/failed state. Similar to how the instance items checkbox colouring works.

@BigRoy
Copy link
Member

BigRoy commented Dec 8, 2017

Yup, you can still do that. The clickable area is the labels boundary. Just the first solution I could think off, so suggestions for better alternatives are welcome.

Ah, I see. Maybe CTRL + click makes more sense to maybe toggle all.

Yeah, I was thinking we could colour the label depending on the succcess/failed state. Similar to how the instance items checkbox colouring works.

Could be a good start yes.

@tokejepsen
Copy link
Member Author

tokejepsen commented Dec 8, 2017

Forgot to show processing:

dec 8 2017 12_42 pm

@mottosso
Copy link
Member

mottosso commented Dec 8, 2017

Wow! This is amazing! There was a request a while back about optionally hiding collectors, as they didn't provide much value to the artist. Maybe some sections could be collapsed per default somehow? Maybe just hardcode the collectors.

@mottosso
Copy link
Member

mottosso commented Dec 8, 2017

Ah, you linked to it, it's #189 :) Perfect.

@tokejepsen
Copy link
Member Author

tokejepsen commented Dec 8, 2017

Maybe some sections could be collapsed per default somehow?

I was thinking we could query for a hidden data member/attribute, similar to how you can set the publish state of an instance. This would makes it very flexible and allow us to hide any section and even let the user customize which section should be hidden.

@BigRoy
Copy link
Member

BigRoy commented Dec 8, 2017

I was thinking we could query for a hidden data member/attribute, similar to how you can set the publish state of an instance. This would makes it very flexible and allow us to hide any section and even let the user customize which section should be hidden.

Actually, it's not similar - because this would be for the complete section not per instance or per plugin. We've never stored data for a grouped section before I believe?

The idea itself though sounds like a good direction.

@davidlatwe
Copy link
Collaborator

Ah, I see. Maybe CTRL + click makes more sense to maybe toggle all.

Just interesting in this :)
Maybe make the v arrow mark into a circular and rotatable checkbox for toggle all ?
Providing both hide/show and all on/off visual hint.

@tokejepsen
Copy link
Member Author

Maybe make the v arrow mark into a circular and rotatable checkbox for toggle all ?

That is an interesting idea :) Never seen something like that before. Slightly worry would be that user aren't familiar with it. Have you got a reference for this from somewhere?

@tokejepsen
Copy link
Member Author

We've never stored data for a grouped section before I believe?

Actually when I think about this some more, it might become too confusing to have some plugins/instance hidden within a section because there is no indication for hidden items.

Maybe its better to expose a public list of sections labels to hide on startup.

import pyblish_qml import settings

settings.HiddenSections = ["Collect"]

@BigRoy
Copy link
Member

BigRoy commented Dec 8, 2017

Maybe make the v arrow mark into a circular and rotatable checkbox for toggle all ?

That is an interesting idea :) Never seen something like that before. Slightly worry would be that user aren't familiar with it. Have you got a reference for this from somewhere?

Also not sure how clear that would be for plug-ins, because there are often only a few which can be toggled. So there would always be a remainder that is enabled - what would it then show? :)

@tokejepsen
Copy link
Member Author

what would it then show?

Good point, that might become confusing.

We could also just rethink where/how to hide a section. I initially thought of have just the arrow > be clickable, but that seemed like a very small area to hit with the cursor.

@davidlatwe
Copy link
Collaborator

davidlatwe commented Dec 8, 2017

Slightly worry would be that user aren't familiar with it.

Yeah, it might :( . Most case I know they always separate the function of tree collapsing and toggling, and visualize both.
Currently only have one persist visual item (v arrow mark) in there to give the collapsible hint, and the toggle all is implicit. Maybe should add one more indicator for keeping those options visualize ?
Or just leave it as is ? Since at least me personally not often need to toggle them all : / sorry, Instances needed.

@davidlatwe
Copy link
Collaborator

We could also just rethink where/how to hide a section.

Yes, right. Maybe use + and - for collapsing ? For differentiate from go to perspective

@tokejepsen
Copy link
Member Author

For differentiate from go to perspective

Good point as well.

Here is an option for separating the collapsing and toggling:

dec 8 2017 2_39 pm

@BigRoy
Copy link
Member

BigRoy commented Dec 8, 2017

Here is an option for separating the collapsing and toggling:

Nice!

This already makes much more sense to me - but yes, the arrow area feels a bit large now; especially in width.

@tokejepsen
Copy link
Member Author

This is starting to feel right :)

dec 8 2017 2_57 pm

@davidlatwe
Copy link
Collaborator

This might be another issue.
For showing the plugin's toggle status if the section is collapsed, how about something like github's modification indicator or deadline's progress bar ?
Darker color would be the can not toggle, mid-gray would be off, white for on, and turn in to progress bar when processing.

status-bar
status-bar-2

maybe won't need number on it, just use color as a simple visual hint.

@tokejepsen
Copy link
Member Author

maybe won't need number on it, just use color as a simple visual hint.

I'm going to try and adhere to the current color scheme and processing indicator. That being when processing the section label will highlight (blink), and on success color the label (and/or the icon) green, on warning=orange and failed=red.

Progress bar is a different issue all together :)

@tokejepsen
Copy link
Member Author

I've hardcoded the "Collect" section to be collapsed. While trying to update the UI from the controller and looking into what is required from getting the colours working when processing, it seems to me that we'll need to have a dedicated "section" in the model.

I'll return to this on Monday, but if anyone has any ideas please chime in.

@tokejepsen
Copy link
Member Author

Why is it taking the "hideState" but not using it?

Nice catch! Fixed it now. Was working because it was assuming the opposite state was desired, which was the case in most scenarios.

@BigRoy
Copy link
Member

BigRoy commented Dec 12, 2017

About the green, would it make more sense to only color a section if it is closed? Then they could be either green or red, as they'd be the only indicator of success or failure.

Hmm, I don't think that's much better. I kind of like it clearly highlighting the status as it is.

@tokejepsen
Copy link
Member Author

About the green, would it make more sense to only color a section if it is closed?

That is interesting. Here it is in practise:

dec 12 2017 4_46 pm

@mottosso
Copy link
Member

I think that looks fantastic. Minimalist, unambiguous. This gets my vote!

@mottosso
Copy link
Member

mottosso commented Dec 12, 2017

Looking back..

I do like the darkness of the sections in this one, and the fact that the button isn't highlighted by default. It's a lot less cluttered.

Let's give this another day or so to sink in before merging.

@mottosso
Copy link
Member

The very first entry is also not bad..

Definitely needs a day or so. :)

@BigRoy
Copy link
Member

BigRoy commented Dec 12, 2017

I'd say let's try to roll out this one: #275 (comment) With the color states only on collapsed.

If that's in, or whatever we decide on for the first version, then I'll roll it out to the team soon and see what they have to say.

@davidlatwe
Copy link
Collaborator

davidlatwe commented Dec 13, 2017

Hi, sorry, just adding one opinion :)

I do like the darkness of the sections in this one

I actually like the brightness of sections stay close with background color though, I think/feel that leaves the main contrast stay on those instances and plugins (gray and white), like introducing this new section style/function in a little smooth way.
But yeah, maybe a day or so, will follow the final vote no matter what !

@mottosso
Copy link
Member

mottosso commented Dec 13, 2017

I've pushed a small change, with a little less contrast.

untitled project

However I do still like the subtleness and consistency of this one.

@BigRoy
Copy link
Member

BigRoy commented Dec 13, 2017

I've pushed a small change.

I'm liking this. :)

@tokejepsen
Copy link
Member Author

Not to fuel the fire, but this is my candidate:

dec 13 2017 10_48 am

This is based on introducing only what is needed:

  • Sections stay the same unless hidden, which is a newly introduced feature, hence needing additional UI features to accommodate.
  • Minimal amount of visuals when sections are hidden. Although colouring just the hide/show button is not enough, but colouring the text compromises the readability of the text.

Direct comparison with current master:

old
new

@mottosso
Copy link
Member

Could you try that @tokejepsen, but without the surrounding rectangle on the section? Just coloring the + button, similar to the checkboxes.

@tokejepsen
Copy link
Member Author

but without the surrounding rectangle on the section?

dec 13 2017 11_09 am

@davidlatwe
Copy link
Collaborator

This is the best to me !!

@mottosso
Copy link
Member

Happy to try this, so we can let it simmer in production for a few weeks and potentially return to this great wall of alternative designs if necessary.

@tokejepsen
Copy link
Member Author

I'd be happy with this as well :)

Least amount of visual change. Then gather opinions from production for further refinement.

@BigRoy
Copy link
Member

BigRoy commented Dec 13, 2017

I'm slightly afraid the warnings might not be visible enough in the last version. And also maybe not clear enough that they are "container headers".

But let's give it a go.

@mottosso
Copy link
Member

Welcome to merge and release (quick! Before someone else comments on it! ;) )

@tokejepsen tokejepsen merged commit d6dcd3d into pyblish:master Dec 13, 2017
@tokejepsen tokejepsen mentioned this pull request Dec 13, 2017
@tokejepsen
Copy link
Member Author

Shit! Too fast, forgot to add the latest changes.

@mottosso
Copy link
Member

Haha! Sorry to rush you! :D

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.

4 participants