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

Add selection CSS classes to tick labels #752

Merged
merged 2 commits into from Nov 25, 2018

Conversation

Projects
None yet
4 participants
@duggi
Copy link
Contributor

commented May 29, 2017

Pull request for #751

--

Adds class label-in-selection if tick labels are in the selection range.
Adds class label-is-selection if tick label is selected, min or max.

--

figure 1. example, selection tick labels are in darker font
image

@seiyria

This comment has been minimized.

Copy link
Owner

commented May 29, 2017

please make sure your code passes our linter

@duggi duggi force-pushed the duggi:enhance-tick-labels branch from 63e3136 to 9d9c0d1 May 29, 2017

@duggi

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2017

resubmitted with compliant style

if (!this.options.range) {
if (this.options.selection === 'after' && percentage >= positionPercentages[0]) {
this._addClass(this.tickLabels[i], 'label-in-selection');
} else if (this.options.selection === 'before' && percentage <= positionPercentages[0]) {

This comment has been minimized.

Copy link
@rovolution

rovolution May 30, 2017

Collaborator

anyway we can add some unit tests for this? something as simple as ensuring the tick label elements that get rendered have the expected class tag?

This comment has been minimized.

Copy link
@duggi

duggi May 31, 2017

Author Contributor

will give it a go. using this in production now so had to create a fork and our own bower package. obvi not ideal so will get this done!

This comment has been minimized.

Copy link
@rovolution

rovolution Jun 4, 2017

Collaborator

@duggi any chance to add the tests? i would love to merge this in!

This comment has been minimized.

Copy link
@rovolution

rovolution Jun 23, 2017

Collaborator

@duggi any follow up on this?

This comment has been minimized.

Copy link
@duggi

duggi Jul 30, 2017

Author Contributor

@rovolution hey sorry for long absence, been busy on other things. could not get the tests authored/working (not my real area of expertise), so put it on hold.

i can ask someone else on my team, or maybe you can give me some quick pointers? i've been working in test/specs/TickMarksSpec.js:

	it("Should show the correct tick labels as 'in-selection', according to the `selection` property", function() {
		var options = {
			ticks: [100, 200, 300, 400, 500],
			value: 250,
			selection: 'after'
		},
		$el = $("#testSlider1");

		testSlider = $el.slider(options);
		expect($el.find('.label-is-selection').length).toBe(1);

		testSlider.slider('destroy');

		options.selection = 'before';
		testSlider = $el.slider(options);
		expect($el.siblings('div.slider').find('.label-is-selection').length).toBe(1);
	});

This comment has been minimized.

Copy link
@rovolution

rovolution Aug 2, 2017

Collaborator

@duggi so based on this example snippet, these would quality as 2 different test cases...one to check if the classes are applied if the selection value is 'after', and one to check if the classes are applied if the selection value is 'before'. Therefore, each of these would be in their own it block.

You would want to have tests for checking if .label-is-selection is applied in the proper cases and if .label-in-selection is applied in the proper cases.

Any common setup (like options) should be moved into a beforeEach function, which will execute before each test is run to ensure the common state is reset properly. Likewise, any common teardown setups should be moved into an afterEach block. See the Jasmine docs for further details

You should be working in the /test/specs/TickLabelSpec.js file, since this is more tick label related. You should nest these under a new describe block so that you can add setup/teardown blocks that only run before your new tests for each use-case.

Your assertion's look fine to me, so I definitely think you are on the right track. Here is what I think it would roughly look like.

/* /test/specs/TickLabelSpec.js */

describe("'.label-is-selection' tests", function() {
 var options;
  var testSlider

 // setup that is executed before each test case in this 'describe' function
 beforeEach(function() {
    var options = {
       ticks: [100, 200, 300, 400, 500],
       value: 250,
       selection: 'after'
    };
  });

 // teardown that is executed after each test case in this 'describe' function
 afterEach(function() {
   testSlider.slider('destroy');
 });

 // test cases
 it("Should show the correct tick labels as 'in-selection' if selection is 'after'", function() {
     // Create a new slider
     var testSlider = $("#testSlider1").slider(options);

    // Assert
   expect($el.find('.label-is-selection').length).toBe(1);
 });

 it("Should show the correct tick labels as 'in-selection' if selection is 'before'", function() {
     // TODO
 });
});

describe("'.label-in-selection' tests", function() {
   // TODO
});

This comment has been minimized.

Copy link
@rovolution

rovolution Aug 18, 2017

Collaborator

@duggi have you had a chance to look at this? Thanks!

This comment has been minimized.

Copy link
@duggi

duggi Sep 5, 2017

Author Contributor

thanks for the detailed example, i'll give it a shot soon. been busy as we're launching a new product line, featuring bootstrap-slider!

this is a priority, since we don't want our app dependent on my fork; just gotta clear out some time to put in some work

This comment has been minimized.

Copy link
@rovolution

rovolution Sep 5, 2017

Collaborator

@duggi thats awesome! if its possible, we would love to see a screenshot or GIF of the slider in your new product! Its always exciting to see how the project is being used :)

This comment has been minimized.

Copy link
@duggi

duggi Sep 7, 2017

Author Contributor

screenshot in my initial comment ... repasted at bottom.

you can also see it live here, click on "Free Consultation" for any advisor. Note this impl is single value only, not range. also uses different styles.

(please don't actually submit a request)

--

image

@jespirit

This comment has been minimized.

Copy link
Collaborator

commented Nov 13, 2018

It's been over a year, so I thought if I could come in and add some tests.

I added unit tests for the label-is-selection and label-in-selection classes.

I may have to do a little refactoring to comply with codeclimate and actually check if the correct tick labels have the correct classes.

@jespirit

This comment has been minimized.

Copy link
Collaborator

commented Nov 13, 2018

How do I deal with the codeclimate.yml file errors?

Do I need to merge in the master branch or rebase onto the master branch to incorporate any new changes since this pull request?

@rovolution

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2018

@jespirit dont worry about the code-climate check. in fact, i will probably go ahead and remove it since if feel it does not really add any value to our project

@seiyria

This comment has been minimized.

Copy link
Owner

commented Nov 14, 2018

@rovolution I got it uninstalled. It shouldn't appear any more!

@rovolution

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2018

@seiyria cool i deleted the config from our repo so it should stop showing up now. @jespirit try rebasing this branch against the latest master to remove the codeclimate check

duggi and others added some commits May 29, 2017

@jespirit jespirit force-pushed the duggi:enhance-tick-labels branch from 74ae19e to a3c645b Nov 14, 2018

@jespirit

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2018

I rebased onto master and added more logic to the unit tests.

@jespirit jespirit merged commit 0fa1ed5 into seiyria:master Nov 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.