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: visibility of icons in IntegerInput #320

Merged

Conversation

kevinjoshi46b
Copy link
Contributor

Solves #319

The input icons now look like this:

* here:
image
image

$, Ξ here:
image
image

and similar for # in byte input

- `#` in `Bytes32Input` & `BytesInput`, `*` in IntegerInput and `$, Ξ`
in `EtherInput` are now clearly visible even in dark mode and display
color set to `text-gray-400`
Copy link
Collaborator

@damianmarti damianmarti left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@carletex carletex left a comment

Choose a reason for hiding this comment

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

@kevinjoshi46b The idea of having daisy themes is to use those colors (primary, secondary, accent, neutral, etc) so they always look consistent if you change your theme colors in tailwind.config.js.

If we hardcode a color, like text-gray-400, you are "disconnecting" it from the theme.

From DaisyUI:
image.

So we need to use a color from the theme (maybe we need to re-think colors in there)

Also, for me, it's not that bad how they look currently:

image

cc @damianmarti @technophile-04

@kevinjoshi46b
Copy link
Contributor Author

If we hardcode a color, like text-gray-400, you are "disconnecting" it from the theme.

Alright ya this is a good point.

Also, for me, it's not that bad how they look currently:

image

Maybe the * is fine because its a more dense icon (but it would probably still go unnoticed maybe on a bigger screen) what about Ξ ?

So we need to use a color from the theme (maybe we need to re-think colors in there)

Ya I do feel this might be needed since the colors are too close and not too contradicting.

Would like to know others opinions too.

@carletex
Copy link
Member

Maybe the * is fine because its a more dense icon (but it would probably still go unnoticed maybe on a bigger screen) what about Ξ ?

If you are refering to the "Ξ" in EtherInput, the problem is that it's using "text-primary" color... and it should be "text-accent" (like the "*" in the value-wei input).

If we want more contrast, we can use "text-neutral".

@technophile-04
Copy link
Collaborator

technophile-04 commented Apr 25, 2023

Maybe the * is fine because its a more dense icon (but it would probably still go unnoticed maybe on a bigger screen)

I feel they are visible in main branch, like I have a kind of big monitor screen and looks great their too...we also had improved their accessibility before check #170

Screenshot 2023-04-25 at 9 41 22 PM

what about Ξ ?

Yeah, as Carlos mentioned nice catch it can be improved and should have been text-accent....lol text-neutral a bit too contrasty and kind off misses the theme so I would go with text-accent but its just my personal preference 😂

@kevinjoshi46b
Copy link
Contributor Author

I feel they are visible in main branch, like I have a kind of big monitor screen and looks great their too...

That's great not an issue there then

text-neutral a bit too contrasty

Ohhh yes, its brighter then the input number itself :)

I majorly noticed this issue in Ξ and then just ended up changing the rest as well assuming they have a similar issue but ya it seems like just changing from text-accent -> text-primary is good enough.

- Reverted rest of the changes and set IntegerInput icons to be
text-primary
@kevinjoshi46b kevinjoshi46b changed the title fix: visibility of icons in input fix: visibility of icons in IntegerInput Apr 25, 2023
@carletex
Copy link
Member

@kevinjoshi46b You did it the other way around.

You changed "text-accent" to "text-primary" in integer & bytes inputs, and left the Ξ as it was. Now I can barely see them.

primary:
image

accent:
image

It's the other way around. You just need to change the Ξ color inside the EtherInput to "text-accent"

@kevinjoshi46b
Copy link
Contributor Author

You changed "text-accent" to "text-primary" in integer & bytes inputs, and left the Ξ as it was. Now I can barely see them.

Oops, my bad sorry for the inconvenience

@carletex carletex merged commit 90b954a into scaffold-eth:main Apr 26, 2023
1 check passed
kmjones1979 added a commit that referenced this pull request Jul 3, 2023
* Update SE-2 nomenclautre  (#317)

* Fix autoConnect when contract not deployed (#292)

* oxford comma lol

* Fix icon visibility on dark mode in EtherInput (#320)

* Update README: Add Vercel ENV var to disable type/lint checking (#327)

* Fix typo (#334)

Fix typo at code snippet

* README: Updated SE-2 custom hook section (#330)

Co-authored-by: KcPele <fidekg123@gmail.com>
Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com>
Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com>

* Remove duplicated HINT on README (#335)

* PR and Issue templates (#329)

* Native currency symbol and price as per target network (#322)

* useAppStore => useGlobalState (#338)

* Event history filter type (#333)

* fix: event history filter type

Fixes #332

* fix: make scaffold event history config separate type

* fix: prettier broke due to invalid syntax

* fix: ts error after generating contract

* Indexed prop + example

---------

Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com>

* Update README: custom hooks section (#339)

* fix: assert type as Abi to avoid ts error (#350)

* fix: assert type as Abi to avoid ts error

Fixes #344

* assert type as Abi to avoid ts error on useScaffoldContract

---------

Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com>

* Fix useEffect filter dep on event hook (#349)

* Block confirmation options / callback for useScaffoldContractWrite (#348)

* Patch useLocalStorage setting the right initial value to avoid a hydration error (#356)

Co-authored-by: sverps <sverps@gmail.com>

* Save selected contract to local storage (#326)

* Save selected contract to local storage

* refactor: Replaced native localStorage with useLocalStorage from usehooks-ts

* Fixed empty string parameter error.

* Updated parameter requirements to fix issue.

* Fixed error.

* Fixed parameter assignment error.

* Use useLocalStorage to save selected contract

---------

Co-authored-by: Damian <damianmarti@gmail.com>

* Refactor meta tags into separate component and add default thumbnail / image (#359)

* Hotfix: thumbnail.png => thumbnail.jpg

* Local Block Explorer (#351)

Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com>

* Fix daisyUI dropdown on IOS (#342)

* allow `overrides` in useScaffoldContractWrite (#345)

* allow overrides for useScaffoldContractWrite

* spread overrides and restConfig separately

* feat: proactively disable read hook when it would result in error (#250)

Fixes #249

Co-authored-by: Shiv Bhonde | shivbhonde.eth <shivbhonde04@gmail.com>

* Size prop on Address component (#365)


Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com>

* Generate TS ABIs from deploy script (#368)

* enable solidity optimizer by default (#360)

* fix(Balance): font-size of symbol should be relative to current font-size, not document root (#375)

* fix(contract.ts): very long abi's now no longer go over type instantiation limit (#377)

Fixes #374

* fix(contract.ts): improve missing config check (#372)

Fixes #371

* fix listner types error when contracts missing (#379)

* zkSyncEra Testnet config (#383)

* Upgrade hardhat-deploy to 0.11.26 where zkSync is supported

* Dependency for @matterlabs/hardhat-zksync-solc

* Add zkSyncEraTestnet to networks

* Import @matterlabs/hardhat-zksync-solc

* Add artifacts-zk and cache-zk to gitignore

* yarn install

* Add zkSync mainnet config

* add hardhat-zksync-verify for contract verification

---------

Co-authored-by: moltam89 <moltam89@gmail.com>
Co-authored-by: Damian <damianmarti@gmail.com>
Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com>

* allow writeAsync by useScaffoldContractWrite to be called with updated args (#385)

* update wagmi & rainbow for walletConnectV2 (#381)

* meged changes from main, additional configuration in subgraph.yaml

---------

Co-authored-by: Shiv Bhonde | shivbhonde.eth <shivbhonde04@gmail.com>
Co-authored-by: Austin Griffith <austin@ethereum.org>
Co-authored-by: Kevin Joshi <kevinjoshi46b@gmail.com>
Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com>
Co-authored-by: AlehN <natsevsky@gmail.com>
Co-authored-by: Pablo Alayeto <55535804+Pabl0cks@users.noreply.github.com>
Co-authored-by: KcPele <fidekg123@gmail.com>
Co-authored-by: Samuel | solidixen.eth <sverps@gmail.com>
Co-authored-by: Eda Akturk <edakturk96@gmail.com>
Co-authored-by: Tamas Molnar <tamas.molnar@liferay.com>
Co-authored-by: Damian Martinelli <damianmarti@gmail.com>
Co-authored-by: Alexander <a00112699@gmail.com>
Co-authored-by: port <108868128+portdeveloper@users.noreply.github.com>
Co-authored-by: moltam89 <moltam89@gmail.com>
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.

None yet

4 participants