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

fix: styles in the Picklist component when the prop readOnly is passed #1684

Merged

Conversation

rgah2107
Copy link
Collaborator

@rgah2107 rgah2107 commented Jul 4, 2020

fix: #1683

Changes proposed in this PR:

@nexxtway/react-rainbow

@commit-lint
Copy link

commit-lint bot commented Jul 4, 2020

Bug Fixes

  • styles in the Picklist component when the prop readOnly is passed (0e9d660)
  • cursor type when hover (b730652)
  • lint problem (5f2d2d3)
  • input component styles related to props readOnly and disabled (6efaece)
  • merge conflict (5793db5)
  • bad behavior when using keyboard navigation (acdd7d9)
  • label style when readOnly and disabled (b0c2357)

Contributors

@rgah2107, @TahimiLeonBravo

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@@ -43,6 +43,7 @@ const PickerInput = attachThemeAttrs(styled(StyledInput))`
box-shadow: none;
background-color: transparent;
border: 2px solid transparent;
pointer-events: none;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this solution is that it changes the course to the hand that indicates some action and the course should be the text

@rgah2107 rgah2107 requested a review from yvmunayev July 7, 2020 22:47
Copy link
Contributor

@yvmunayev yvmunayev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y
Screen Shot 2020-07-10 at 4 37 26 AM
ou have to take into account keyboard navigation

@@ -29,6 +29,7 @@ Label.propTypes = {
required: PropTypes.bool,
inputId: PropTypes.string,
readOnly: PropTypes.bool,
disabled: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To change the API of a component a previous discussion is necessary, this small change has a big impact, you had to change 5 component which in turn many more components depends on them for this reason it is not feasible. I think the best solution would be to add to the RenderIf where the Label is painted the conditions of disabled and readOnly

@TahimiLeonBravo
Copy link
Collaborator

TahimiLeonBravo commented Jul 10, 2020

Y
Screen Shot 2020-07-10 at 4 37 26 AM
ou have to take into account keyboard navigation

we have a bug when use the keyboard navigation, you can review the bug on the video here
https://drive.google.com/file/d/1jN8zhbmB-UNdEoDOmtImcbKjjKYXeQG8/view?usp=sharing

@rgah2107 rgah2107 requested a review from yvmunayev July 11, 2020 13:46
@TahimiLeonBravo
Copy link
Collaborator

@rgah2107 the label should be centered on the Picklist component when readOnly and disabled are passed at the same time
Screen Shot 2020-07-12 at 5 06 16 PM

@codeclimate
Copy link

codeclimate bot commented Jul 12, 2020

Code Climate has analyzed commit 58146c0 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Collaborator

@TahimiLeonBravo TahimiLeonBravo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@TahimiLeonBravo TahimiLeonBravo merged commit 3a2069f into nexxtway:master Jul 13, 2020
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

Successfully merging this pull request may close these issues.

fix: Picklist and Input component styles when readOnly
4 participants