Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

PaperUI: rework bindings list view & binding detail view #3782

Merged
merged 2 commits into from Jul 26, 2017

Conversation

htreu
Copy link
Contributor

@htreu htreu commented Jun 30, 2017

Also: refactor list views.

@cdjackson I took you work from #3237 and reworked the bindings view a little. The expansion panels are now used for the thing types and the bindings are listed instead of cards. Let me know what you think.

Here are some screenshots:

Binding list:
screen shot 2017-06-30 at 11 35 44

Sonos binding, editable and more then three supported things with filter:
screen shot 2017-06-30 at 11 36 01

Sonos binding with filter applied:
screen shot 2017-06-30 at 11 36 31

Astro binding w/o configuration and filter:
screen shot 2017-06-30 at 11 36 47

@cdjackson
Copy link
Contributor

I think if people work on other peoples PRs it would be a good idea to make a comment so we don't all waste time working on the same thing......

Anyway...

So you have now removed the extension panel for the overview? What happens if you have a large overview - wasn't this the purpose of the extension panels?

I personally don't like the borders around the supported things list, but that's just personal preference maybe.

Otherwise looks fine to me. Shall I close the other PR?

@kaikreuzer
Copy link
Contributor

it would be a good idea to make a comment

It was actually me that did the comment on #3237: "I'll try to come up with some concrete changes"
@htreu Only helped my first prototyping these changes and then noticed that it all ends up in a bigger refactoring, so I have asked him to do a new PR as a replacement for #3237, which addresses all the infrastructure parts at the same time.

What happens if you have a large overview - wasn't this the purpose of the extension panels?

No, see #3237 (comment). Excessive documentation should rather be put on the official docs page (the README.md of the binding).

I personally don't like the borders around the supported things list

I assumed that this is a fixed design of the extension panels. If it is possible to change, I would also prefer a different, more Material-Design-like design.

@kaikreuzer
Copy link
Contributor

Only helped my first prototyping these changes

Just noticed that this sentence does not pay the due respect. It was actually him, who had the good ideas how to get the design of all our different pages and lists to a common approach to have an overall better UX. I, as usual, only asked stupid questions inbetween :-)

@htreu
Copy link
Contributor Author

htreu commented Jun 30, 2017

@cdjackson & @kaikreuzer

I personally don't like the borders around the supported things list

and

I would also prefer a different, more Material-Design-like design

What do you mean by "border" and "more MD-like"? In the guidelines https://material.io/guidelines/components/expansion-panels.html# the expansion panel is always bordered. If you refer to the whitespace left and right to the panel its due to the fact that the whole content in the binding details view is surrounded by a border. If we change the expansion panel to float from left to right w/o borders it will break the whole column with overview and "Supported Things" sub-headline.

only asked stupid questions inbetween :-)

not at all ;-)

@cdjackson
Copy link
Contributor

Ok, I just comment that it would be useful to say that someone else is working on a PR as I also have spent some more time on this while waiting for feedback to my questions and I just prefer to avoid wasting time if someone else has take over the development.

@htreu
Copy link
Contributor Author

htreu commented Jun 30, 2017

@cdjackson agreed. we started the discussion when your PR was closed and in the beginning it was just meant to be a prototype. Then the scope got too big to make a PR against your work. Next time I will leave you a message.

@cdjackson
Copy link
Contributor

Ok, I will close my PR.

@kaikreuzer
Copy link
Contributor

Next time I will leave you a message.

But note that it is less than 24 hours ago that all of that was done - so a message wouldn't have changed much. So @cdjackson, if you have pending work on PRs that isn't publicly visible yet, better leave a message ;-)

@kaikreuzer
Copy link
Contributor

What do you mean by "border" and "more MD-like"?

Looking at your link, I would say that if we changed the background from white to light-gray, it would perfectly match the MD-examples. I am aware that this comment might cause quite some further big refactorings, though 😎

@htreu
Copy link
Contributor Author

htreu commented Jun 30, 2017

I´m fine with that 👍
But I would suggest to create another issue for that. There are quite some places on PaperUI which can be optimised against the MD guidelines. Regarding the list views we have: I would love to see them as a flexible component (aka directive) and then refactor this directive into something more MD like (and doing the css stuff only once).

@kaikreuzer
Copy link
Contributor

Sure, a follow-up PR will do!

@kaikreuzer
Copy link
Contributor

Visually, this PR then looks good for me, wrt the code, I would leave the review to someone else, though.

@cdjackson
Copy link
Contributor

if you have pending work on PRs that isn't publicly visible yet, better leave a message

Ok, but for me the person creating the PR should retain the baseline unless discussion presents that someoene else will take over or something. Maybe here it's a bit messy as I accidentally closed the PR, but I made changes a week back as per my questions to you and there was no response until this PR.

I don't meant to be a pain, but I also don't want to waste my time, or anyone elses....

@kaikreuzer
Copy link
Contributor

I don't meant to be a pain, but I also don't want to waste my time, or anyone elses....

Clearly nobody wants, so accept my apologies if that has happened. Instead of requiring more of your time by making fuzzy comments, I through it would help to come around with real code as a PR against your PR to address my concerns - I would consider this a usual practice.

@cdjackson
Copy link
Contributor

That's fine, but I just ask that someone make a comment so that we don't end up with multiple people working on the same PR as we have here. You guys clearly benefit from other communications channels so I just ask that if someone takes over someone elses PR that there's a comment/discussion at least.

@kaikreuzer
Copy link
Contributor

As I said above: I HAD left a comment and promised to come up with something - so I assumed the ball was on my side. I'll try to express myself clearer the next time!

@kaikreuzer
Copy link
Contributor

if someone takes over someone elses PR

It was not meant a takeover, but a contribution to it, which became bigger and bigger.

Also: refactor list views

Also-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
@htreu
Copy link
Contributor Author

htreu commented Jul 10, 2017

@cdjackson I know the timing and communication was not ideal, but do you mind taking a closer look on this one? Would be really nice.

@@ -12,6 +12,7 @@
"angular-animate": "1.4.8",
"angular-aria": "1.4.8",
"angular-material": "1.0.1",
"angular-material-expansion-panel": "^0.7.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that for stuff that we package we have to stick to the exact version, only for build&test dependencies like gulp we can go higher.

…dencies to devDependencies

Signed-off-by: Henning Treu <henning.treu@telekom.de>
@htreu
Copy link
Contributor Author

htreu commented Jul 25, 2017

we have to stick to the exact version

done. I did not touch "eventsource-polyfill": "^0.9.6", and "jquery-ui": "^1.11.2", here.

See #3889.

@kaikreuzer
Copy link
Contributor

Thanks! Imho good to merge.
@cdjackson If you have any further comments, let's address them in follow up PRs.

@kaikreuzer
Copy link
Contributor

I´m fine with that 👍
But I would suggest to create another issue for that.

-> #3893

@kaikreuzer kaikreuzer merged commit 5c4a676 into eclipse-archived:master Jul 26, 2017
@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants