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

Flat mode doesn't work #90

Closed
seldary opened this issue Apr 30, 2013 · 10 comments
Closed

Flat mode doesn't work #90

seldary opened this issue Apr 30, 2013 · 10 comments

Comments

@seldary
Copy link

seldary commented Apr 30, 2013

http://jsfiddle.net/XEF6F/1/

Things that don't work:

  • Showing the palette
  • Change event
  • "Choose" button
    and more...
@bgrins
Copy link
Owner

bgrins commented Apr 30, 2013

  • Showing the palette: That option doesn't seem to be enabled in your demo. It seems to be working for me on this demo: http://jsfiddle.net/XEF6F/2/.
  • Change event: this is tricky - change has never really fired on flat pickers. I've always used move instead. We could try and make it more consistent with the normal mode, but since there isn't really a time when you exit the flat colorpicker, it hasn't been implemented.
  • Choose / cancel have odd semantics on a flat picker. We could show the buttons, but what is the intended behavior when you press cancel or choose and the colorpicker is still visible?
  • What else isn't working?

I wouldn't be opposed to making some changes to have the flat mode be more consistent. If you could explain your expected behavior / functionality especially for change, "Choose" and "cancel" we could talk about ways to implement it. Alternatively, this could be a case where I need to add clearer documentation about some of limitations of flat mode.

@seldary
Copy link
Author

seldary commented Apr 30, 2013

Sorry - I may have been wrong about the palette... This is what I had initially, and it worked:

$("#colorPicker").spectrum({
color: self.store.themeColor(),
clickoutFiresChange: true,
showPalette: true,
showInitial: true,
chooseText: "Apply",
change: function(color) {
console.log(color);
var newColor = color.toHexString();
// Do something with newColor
}
});

When I added "flat: true":

  • the "change" event stopped working
  • "clickoutFiresChange" stopped working
  • The "Apply" linked disappeard

I now understand your reasons, but I propose a documentation change on the "flat" parameter...
Also, I would suggest support for "change". If you remove the "cancel" button it seems legit.

@bgrins
Copy link
Owner

bgrins commented Apr 30, 2013

Cool, I think it would be a good addition to put the choose button / chooseText back in on the flat colorpicker (possibly with the requirement that you explicitly set showButtons: true). When the choose button (or a palette swatch) is clicked, it would fire a change event.

Adding some documentation about the differences between the two modes would be good too. As far as clickoutFiresChange, I think this is still not going to not have any meaning on a flat colorpicker - which can be added to the docs.

@kepi
Copy link

kepi commented May 5, 2013

+1 for showButtons true and change event.

@bgrins
Copy link
Owner

bgrins commented May 5, 2013

I have been working on implementing this over on the issue90 branch. Here is a demo page: http://jsbin.com/asafur/3/edit.

Can you try grabbing https://github.com/bgrins/spectrum/blob/issue90/spectrum.css and https://github.com/bgrins/spectrum/blob/issue90/spectrum.js and let me know if it works for you in your projects? Seems like palette and 'choose' button are working fine (I'm hiding the cancel button for now as it seems like the meaning of it is a little confusing on a flat picker).

@kepi
Copy link

kepi commented May 6, 2013

It works exactly like I need, thanks!

As for cancel button, I don't need it absolutely but it really has it's meaning in some scenarios. In my case I'm opening page with flat picker with some selected color. When I try to pick some different color, I may need to return back to original one. Better name for button will be "reset" in this case but this we have already option to change.

@bgrins
Copy link
Owner

bgrins commented May 7, 2013

I am fine with leaving the cancel button in, then. We could provide instructions for how to hide it with CSS if needed. I am still wondering if we should make it so that showButtons needs to be explicitly set to true. It would be a little bit odd that the defaults change for flat, but I am thinking it might be the most appropriate default. Any thoughts on that?

@kepi
Copy link

kepi commented May 7, 2013

I'm really not sure what is best approach. One things is to preserve current system (in flat default is not to shown buttons) and another thing is clearness for future.

IMHO it is best to have same default for all modes and document this in changelog.

@bgrins
Copy link
Owner

bgrins commented May 7, 2013

@kepi I tend to agree with you after looking through the code and seeing the changes I would need to make in order to support the new behavior. Probably best to just stay consistent.

@bgrins
Copy link
Owner

bgrins commented May 10, 2013

@kepi @seldary this is now in master.

@bgrins bgrins closed this as completed May 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants