-
-
Notifications
You must be signed in to change notification settings - Fork 399
Implemented RTL feature. #245
base: develop
Are you sure you want to change the base?
Conversation
Hi, can anybody please go through this PR |
@NayanKhedkar first of all thanks for ur PR! I would prefer a solution where u can set an attribute for the direction. To be more flexible and e.g. We should also consider the orientation of the element in combination with the direction. |
@andreruffert hearty thanks! But I think this feature not implemented yet in rangeslider.js . |
@NayanKhedkar yep the feature is not implemented so far. I was thinking about if you could refactor your implementation like I described above to be more flexible by using
instead of
|
@andreruffert sure . |
@andreruffert as you suggested I refactored implementation on the same PR . Supported data-directions are Please look at once to this PR,and if you have any additions, corrections or suggestions, please let me know. |
dist/rangeslider.css
Outdated
/*top-to-bottom*/ | ||
.rangeslider__ttb { | ||
top: 0; | ||
bottom: initial; |
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.
initial
isn't supported in Internet Explorer.
@andreruffert I would appreciate that you're busy at the moment. As you said I made the changes. Could you please look into the changes? |
@NayanKhedkar It's very hard to diff the style, as you've added two extra spaces to every single line, whether or not it was modified. Can you revert that change and follow the existing file's coding style of only using 2 spaces for indentation instead of 4? That will make the diff much smaller and focus only on your actual changes. Not only that, but there's an extra space (9 in total) on this line: https://github.com/andreruffert/rangeslider.js/pull/245/files#diff-21d11fb857fabcd9805d3e86413a1c49R453 |
@peteygao Thanks , I have removed the extra spaces.Could you please review the changes . . .? |
* @param {String} element | ||
* @param {String} orientation | ||
*/ | ||
function getDirection(element,orientation){ |
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 you follow the rest of the file's formatting style for new code you've written? Namely, please have spaces between symbols, commas, and other delimiters. Example for this function:
function getDirection(element, orientation) {
var direction = element[0].getAttribute('data-direction') || (orientation === 'vertical' ? 'btt' : 'ltr');
if (constants.orientation[orientation].direction[direction]) {
return direction;
} else {
return orientation === 'vertical' ? 'btt' : 'ltr';
}
}
@peteygao Hi, I hope so this time is better. |
Hi, I'm looking forward to feedback. |
@andreruffert @peteygao how is this looking now? keen to get this merged as we use this excellent library in https://github.com/adaptlearning/adapt-contrib-slider and would love to have it updated properly rather than having to hack our copy! |
dist/rangeslider.js
Outdated
* @param {String} element | ||
* @param {String} orientation | ||
*/ | ||
function getDirection(element,orientation){ |
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.
@NayanKhedkar, can you correct the spacing in this function?
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.
👍
@NayanKhedkar Thank you so much!!!! <3 |
How's it looking for a merge for this? On a sidenote: The data-direction method is nice, but a programmatic way to set the direction either on init or via method would be very useful as well. |
Thanks for the hard work! Any chance for a merge on this at some point soon? |
Great work on all of this! Is there any status on when this will be merged and released? |
$r.rangeslider({
polyfill: false,
isRTL:true,//set this property to true for RTL feature
onSlide:onSlideEnd
});