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

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

Merged
merged 2 commits into from Jul 26, 2017

Conversation

Projects
None yet
4 participants
@htreu
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2017

Ok, I will close my PR.

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

Sure, a follow-up PR will do!

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

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

@cdjackson

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

if someone takes over someone elses PR

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

PaperUI: rework bindings list view & binding detail view
Also: refactor list views

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

@htreu htreu force-pushed the htreu:3237-binding-ui branch from 9f07f1b to ff389a4 Jul 10, 2017

@htreu

This comment has been minimized.

Copy link
Contributor Author

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",

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Jul 24, 2017

Member

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.

Henning Treu
force strict angular-material-expansion-panel version, move dev depen…
…dencies to devDependencies

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Jul 26, 2017

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

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Jul 26, 2017

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

-> #3893

@kaikreuzer kaikreuzer merged commit 5c4a676 into eclipse:master Jul 26, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.