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

[DDW-337] fix select arrow styling to allow color overrides #68

Merged
merged 6 commits into from
Jul 6, 2018

Conversation

DominikGuzei
Copy link
Member

@DominikGuzei DominikGuzei commented Jun 28, 2018

This PR fixes the css implementation of the select arrow to allow to define the color of the icon without having to reach into the SVG (which is not possible if only rendered with CSS because it's not part of the DOM).

Additional changes:

  • Fixed vertical positioning of the select arrow in opened state
  • Fixed bubble positioning logic for selects that open upward

@DominikGuzei DominikGuzei self-assigned this Jun 28, 2018
@DominikGuzei DominikGuzei changed the title [DDW-337] improve select arrow styling options [DDW-337] fix select arrow styling to allow color overrides Jun 28, 2018
@nikolaglumac
Copy link
Contributor

@DominikGuzei is this PR ready for review?

@DominikGuzei
Copy link
Member Author

@nikolaglumac yes but i wanted to test it with Daedalus before … btw. this is the issue with only using yarn - it doesn't work with linking the package locally / over branches. For that you still need to use npm (but you can simply not add the lock file to the repo)

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

@DominikGuzei I have tested your fix in Daedalus and I can confirm that now the inactive state is fine (arrow is correctly positioned and has correct color). However, when the select is active/open then the arrow is no longer vertically centered and has wrong color (the color part could be that we are just missing the correct css variable on Daedalus side). Thanks for looking into this!

Current state
wrong

Expected state
right

@DominikGuzei
Copy link
Member Author

@nikolaglumac yes this PR was never integrated into Daedalus ;) will look into that now

@DominikGuzei DominikGuzei removed the WIP label Jul 5, 2018
@DominikGuzei
Copy link
Member Author

DominikGuzei commented Jul 5, 2018

@nikolaglumac @MarcusHurney i fixed the small issue with the vertical centering of the select arrow … and on the way i found a much bigger issue with broken bubble positioning for selects that open upward. The reason basically is that the previous bubble code assumed that it can always position itself based on the parent node … which is very fragile because (as it obviously happened in a recent refactor to refs) the DOM of the parent can change in many ways.

So i introduced a new (optional) targetRef for bubbles that can point to any DOM element provided by the outer components … and if given the bubble targets that element for positioning. This way the select passes down the inputRef and thus the bubble positions itself correctly again 😉

@DominikGuzei
Copy link
Member Author

@nikolaglumac you can also test the fix in Daedalus now: input-output-hk/daedalus@73abd1d

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

Great work @DominikGuzei 🎉
I have tested it in Daedalus (as you suggested) and it is working perfectly 👌

@nikolaglumac nikolaglumac merged commit 05071e3 into develop Jul 6, 2018
@nikolaglumac nikolaglumac deleted the fix/ddw-337-inline-select-arrow-svg branch July 6, 2018 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants