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

feat: Implement dark theme for Select and TextField #366

Merged
merged 6 commits into from
Oct 7, 2021

Conversation

dequejosie
Copy link
Contributor

@schne324
Copy link
Member

schne324 commented Oct 6, 2021

Haven't had a chance to review the code changes, but I just looked at the deploy preview and it looks like the text field (including multiline) has the wrong font color set:

Screen Shot 2021-10-06 at 2 53 16 PM

Screen Shot 2021-10-06 at 2 53 01 PM

The uxpin documents the color as #ffffff

Copy link
Member

@schne324 schne324 left a comment

Choose a reason for hiding this comment

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

Font color issue

}

.cauldron--theme-dark input,
select,
Copy link
Member

Choose a reason for hiding this comment

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

Are we intentionally applying styles to all <select> elements here? IMO Cauldron should only style elements it renders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. The code has been there and I was assuming that it only affects Cauldron components, but now I think about it. It probably will affects these native elements outside Cauldron too, no? @schne324

Copy link
Member

Choose a reason for hiding this comment

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

@stephenmathieson is correct - cauldron should only style its components. Given that this was already there, I'm cool if we want to address this issue in a separate PR

Copy link
Member

Choose a reason for hiding this comment

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

Digging deeper, this file is full of cauldron styles that'd apply to non-cauldron components (see https://github.com/dequelabs/cauldron/pull/366/files#diff-d2f5588b0cde6a6cbe8f128481e2f436d72c46d49ecd4a507c36b28f001a9b6dR34-R42

Copy link
Member

Choose a reason for hiding this comment

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

☝️ more of a reason to address this issue separately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue for this

@dequejosie dequejosie merged commit 8f02a50 into develop Oct 7, 2021
@dequejosie dequejosie deleted the dark-theme-select branch October 7, 2021 16:57
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.

3 participants