Skip to content

Use select componsition to style pf3 select #272

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

Merged
merged 14 commits into from
Jan 7, 2020
Merged

Use select componsition to style pf3 select #272

merged 14 commits into from
Jan 7, 2020

Conversation

Hyperkid123
Copy link
Member

@Hyperkid123 Hyperkid123 commented Dec 18, 2019

Changes

  • use traditional css styling instead of css in js
  • use composite components
  • the searchable variant is now using PF dropdown as a base of the select to avoid positioning the input by css
  • main logic was moved to common package
  • async has to stay package-specific to due to the dropdown base (sorry can't do anything about that) and constant re-initializing if the component in searchable variant
  • fixed some PF styling inconsistency

@rvsia @skateman this was a lot of refactoring and lof of the internal logic has changed. I would appreciate it if you could test this very thoroughly since there is a high chance that I've missed something. Even though the tests might pass I would like to test it manually.

@Hyperkid123 Hyperkid123 force-pushed the common-select branch 2 times, most recently from ff61483 to c204f9e Compare December 19, 2019 11:54
@Hyperkid123 Hyperkid123 changed the title [WIP] Use select componsition to style pf3 select Use select componsition to style pf3 select Dec 19, 2019
@Hyperkid123 Hyperkid123 requested a review from skateman December 19, 2019 14:40
@codecov
Copy link

codecov bot commented Dec 19, 2019

Codecov Report

Merging #272 into master will increase coverage by 0.9%.
The diff coverage is 92.56%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #272     +/-   ##
=========================================
+ Coverage   81.54%   82.45%   +0.9%     
=========================================
  Files          88       93      +5     
  Lines        1783     1824     +41     
  Branches      604      607      +3     
=========================================
+ Hits         1454     1504     +50     
+ Misses        329      320      -9
Impacted Files Coverage Δ
...f3-component-mapper/src/form-fields/form-fields.js 95.34% <ø> (ø) ⬆️
packages/common/src/utils/fn-to-string.js 100% <100%> (ø)
...t-mapper/src/form-fields/select/clear-indicator.js 100% <100%> (ø)
...apper/src/form-fields/select/dropdown-indicator.js 100% <100%> (ø)
...-component-mapper/src/form-fields/select/option.js 100% <100%> (ø)
packages/common/src/prop-types-templates.js 100% <100%> (ø)
...3-component-mapper/src/form-fields/select/index.js 92.47% <91.35%> (-3.45%) ⬇️
packages/common/src/select/index.js 92.3% <92.3%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf5b39b...333b9b6. Read the comment docs.

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval
Tested in DDF editor, no issues 👍

Copy link
Contributor

@rvsia rvsia left a comment

Choose a reason for hiding this comment

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

Looks great! 🏆

@rvsia
Copy link
Contributor

rvsia commented Jan 6, 2020

@Hyperkid123 Looks great, however, the code coverage decreased too much. Could you add some test to increase it please?

@Hyperkid123
Copy link
Member Author

i will once i am able

@rvsia rvsia merged commit 83cff51 into master Jan 7, 2020
@Hyperkid123 Hyperkid123 deleted the common-select branch January 7, 2020 12:52
@Hyperkid123
Copy link
Member Author

🎉 This PR is included in version 1.25.0 🎉

The release is available on

Demo can be found here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants