-
Notifications
You must be signed in to change notification settings - Fork 5
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
MBS-8586: Simplify horizontal scrolling #34
Conversation
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.
Looks good in general.
Some minor questions/thoughts/suggestions:
- Would it be a good idea to make the steps when clicking the buttons dependent on the card with? Currently it feels a bit like I have to click too often to move right, so maybe we can use cardwith/2 as scroll length or even 0.9*cardwith or something?
- To fit into the overall design maybe we could style the buttons with rounded corners.
- It would be nice if the buttons could be transparent, so when they are shown, you can still see the content of the card behind them
- User might eventually expect that I can keep scrolling by keeping the buttons pressed. Is this something we want?
styles.css
Outdated
|
||
.mod_kanban_scroll_right { | ||
left: calc(100vw - 30px); | ||
} |
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.
missing new line
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.
still no newline
2a223fe
to
7b396a6
Compare
Introducing transparency produces a quite messy output - maybe we'll find another approach for implementing |
7b396a6
to
0e33f83
Compare
0e33f83
to
5506e84
Compare
amd/src/board.js
Outdated
} else { | ||
this.getElement(selectors.SCROLLLEFT).style.setProperty('visibility', 'visible'); | ||
} | ||
if (main.scrollLeft < main.scrollLeftMax) { |
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 a standard property, does not work on Chrome and many other browsers, see: https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollLeftMax?retiredLocale=de
Needs to be changed.
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.
Pls see comments.
5506e84
to
48b3ebe
Compare
Accidentally pushed wrong version - sorry for that! |
48b3ebe
to
34a7e55
Compare
386a094
to
8a80298
Compare
8a80298
to
9b283d9
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.
Great feature, well done!
Internally reviewed, happy to merge.
Some more improvements regarding button styling and general handling will be introduced in a separate PR.
No description provided.