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

focusStyle, hoverStyle, activeStyle and so on should extend not overwrite style #60

Closed
5 tasks done
nikgraf opened this issue Jul 5, 2015 · 7 comments
Closed
5 tasks done

Comments

@nikgraf
Copy link
Owner

nikgraf commented Jul 5, 2015

Same for disabledHover. It should extend disabledStyle.

I noticed I made this mistake at the Select. There might be other places. We should double-check.

  • fix hoverStyle behaviour in Select
  • fix focusStyle behaviour in Select
  • deprecate duplicated styles definitions in style.select.hoverStyle, style.select.activeStyle, style.select.focusStyle
  • investigate if other components have the same issue
  • update documentation to explain how psuedoClass styles work in Belle
@nikgraf
Copy link
Owner Author

nikgraf commented Jul 7, 2015

We probably should also make that clear in the documentation as well (every component).

Could be done with an example like:

// When hovering the component, the color will be blue and the background still green.
<Button style={{ color: red, background: green }} hoverStyle={{ color: blue }}>Follow</Button>

@jpuri jpuri self-assigned this Jul 8, 2015
@jpuri
Copy link
Collaborator

jpuri commented Jul 9, 2015

Our implementation of Rating does not have a hoverStyle or disabledHoverStyle....hover for Rating is implemented at level of character and not wrapper, thus we have this: hoverCharacterStyle.

Its look fine in terms of logic, but api looks different from other components.

@jpuri
Copy link
Collaborator

jpuri commented Jul 9, 2015

Why should disabledHoverStyle extend disabledStyle ?
Former is applied as pseudo class.

@nikgraf
Copy link
Owner Author

nikgraf commented Jul 9, 2015

@jpuri by extend I meant it like this:

// When hovering the diabled component, the color will be blue and the background still green.
<Button disabledStyle={{ color: red, background: green }} disabledHoverStyle ={{ color: blue }}>Follow</Button>

Regarding the API: Would you suggest to change the API for Rating? While it doesn't comply with the other components styling configuration I though it would make more sense as the style is directly applied to the character. Maybe the old html structure was better?

@jpuri
Copy link
Collaborator

jpuri commented Jul 9, 2015

No this structure is good....its just about how we have grouped and designed the styles.
We can change our styling api for Rating to comply with other components and still keep same structure...

I see the list of stuff to be completed for release just growing as we are working on things... :)

@nikgraf
Copy link
Owner Author

nikgraf commented Jul 9, 2015

👍 sure, I added this issue: #86

@nikgraf
Copy link
Owner Author

nikgraf commented Jul 9, 2015

Solved with this PR: #87

@nikgraf nikgraf closed this as completed Jul 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants