-
Notifications
You must be signed in to change notification settings - Fork 275
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
Simplify board view #199
Simplify board view #199
Conversation
Codecov Report
@@ Coverage Diff @@
## master #199 +/- ##
=======================================
Coverage 76.56% 76.56%
=======================================
Files 33 33
Lines 973 973
=======================================
Hits 745 745
Misses 228 228 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't delete a stack. Delete-icon is no more visible on mouse over. |
css/style.css
Outdated
@@ -286,6 +299,10 @@ button.button-inline:hover { | |||
overflow: hidden; | |||
text-overflow: ellipsis; | |||
} | |||
.stack ul.card-list { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for both ul
and .card-list
- you can target this element specifically with .stack > ul
or simply .card-list
. Best not to have too many queries in CSS
css/style.css
Outdated
background-color: #f8f8f8; | ||
background-color: #ffffff; | ||
border-right: 1px solid #eee; | ||
position: relative; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
css/style.css
Outdated
#board #innerBoard { | ||
padding: 10px; | ||
#innerBoard { | ||
padding: 0 10px 20px 10px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why complicate padding? You essentially just move top padding over to margin-top for #board
.
Edit: in fact, you could remove the whole declaration by just giving padding: 10px
to #board
element
css/style.css
Outdated
@@ -125,10 +125,11 @@ button.button-inline:hover { | |||
width: 100%; | |||
bottom: 0px; | |||
top: 44px; | |||
margin-top: 10px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
margin for a fixed
element? Why not just modify top
value?
css/style.css
Outdated
@@ -264,6 +273,10 @@ button.button-inline:hover { | |||
overflow: hidden; | |||
display: flex; | |||
min-height: 40px; | |||
position: fixed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why fixed position? It just creates problems:
-
you have to give "imaginary"
margin-top
to.card-list
(line 303). Everything seems to work just as fine without those 3 declarations -
mobile issue mentioned by @artemanufrij
It works just fine if you remove those 3 lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind that was to keep the card title always on top even when scrolling though larger task lists. But I guess I'll remove this for now and try to add this in a separate PR. I guess it will also require some javascript magic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that was a good idea, but yeah, probably some JS will be needed.
I'm more worried about how it would look though. Fixed titles usually require some extra style to distinguish. from the rest of the DOM
css/style.css
Outdated
@@ -441,7 +459,7 @@ button.button-inline:hover { | |||
.card.create { | |||
text-align: center; | |||
padding: 10px; | |||
margin: 10px; | |||
margin: 10px 10px 0 10px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any particular reason for removing bottom margin on this button?
css/style.css
Outdated
.stack { | ||
width: 320px; | ||
min-width: 320px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't didn't you mention resizing from 200px? Left like this, it has 0 effect because of width: 320px
css/style.css
Outdated
display: inline-block; | ||
position: relative; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required - since no positioning values (e.g. top
) are applied to this class.
css/style.css
Outdated
vertical-align: top; | ||
background-color: #f8f8f8; | ||
background-color: #ffffff; | ||
border-right: 1px solid #eee; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
border should not be visible on mobile
css/style.css
Outdated
vertical-align: top; | ||
background-color: #f8f8f8; | ||
background-color: #ffffff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.stack
doesn't need background-color if it's white, right?
5285b29
to
87df804
Compare
87df804
to
abf1f15
Compare
@pixelipo Updated and removed the fixed header stuff. I'm not sure if the dynamic sizing proposed in #82 of stacks makes much sense. I don't see much value, since it does only depend on the number of stacks not the browser width. What do you think, @artemanufrij @pixelipo ? |
I don't like think about dynamic size. I realy like current style (this PR). I vote for fix width for stacks 👍 ! |
abf1f15
to
544a494
Compare
544a494
to
752b4a8
Compare
I agree with Artem. Only a single element of the card would benefit from dynamic width - card title. Everything else takes up already existing space. It would look real bad on large screens |
9fb0a16
to
acdfd6b
Compare
Signed-off-by: Julius Härtl <jus@bitgrid.net>
acdfd6b
to
73139c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed all issues as discussed in irc! great work!!!
Looks much nicer and lighter without the background, good job! :) |
Fixed stack headers, so it is clear where you are when scrollingFeedback is very welcome. I'm not sure if the resizing of the stack width is a good way or if we should just go for a fixed width there.