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

Update CSS Styling #186

Closed
wants to merge 1 commit into from
Closed

Conversation

death7654
Copy link
Contributor

@death7654 death7654 commented Jul 10, 2024

-Make Dark Mode Darker
-Add A Placeholder To The Search Bar In Supported Devices
-Match Search Bar Styling to Nav Search Bar in Supported Devices
-Make Danger Tip Have Better Contrast
-Make Nav Items Larger for better Accessibility

Copy link
Member

@ninelore ninelore left a comment

Choose a reason for hiding this comment

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

Just like the other PR: Blocking for Lighthouse checks

@death7654 death7654 reopened this Jul 10, 2024
@death7654 death7654 requested a review from ninelore July 10, 2024 07:51
Copy link
Member

@ninelore ninelore left a comment

Choose a reason for hiding this comment

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

See above

@death7654 death7654 force-pushed the darkmode branch 2 times, most recently from b3d059c to 99b9098 Compare July 10, 2024 10:48
@death7654
Copy link
Contributor Author

Is it possible to bypass Lint checks for this PR as, it is only a CSS update, and not added content

@ninelore
Copy link
Member

Checks dont block merge

-Make Dark Mode Darker
-Add A Placeholder To The Search Bar In `Supported Devices`
-Match Search Bar Styling to Nav Search Bar in `Supported Devices`
-Make Danger Tip Have Better Contrast
-Make Nav Items Larger for better Accessibility
-Change Code Snippet Colors for better contrast
@@ -40,7 +40,7 @@ If you're on a smaller screen, scroll sideways to see whole table.
:::

<AddScript script-url="../../supported-devices.js"/>
<p>Search: <input type="text" class="deviceSearch"></p>
<p><input type="text" class="deviceSearch" placeholder="Enter Your Boardname Or Device Name"></p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

removing the 'Search' prefix, coupled with the css color changes, makes it almost impossible to tell that there is a search bar there now:

image

the old version was much better:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change the bg and make it pop more

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's one example of a larger problem IMO. I don't know what issue this PR is trying to solve

Copy link
Member

Choose a reason for hiding this comment

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

The field and text are a accessibility issue aswell due to missing contrast

@MrChromebox
Copy link
Collaborator

the CSS changes here overall reduce the contrast between elements in dark mode, making things harder to see. I can't say I'm a fan at all.

@death7654
Copy link
Contributor Author

Is this a reference to the search bar or the website as a whole?

@MrChromebox
Copy link
Collaborator

Is this a reference to the search bar or the website as a whole?

both. What is the goal of the CSS changes in this PR?

.sidebar::-webkit-scrollbar {
width: 0%;
height: 0%;

Copy link
Member

Choose a reason for hiding this comment

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

Why no scrollbar (in the first place)? This is a accessibility issue...

@@ -40,7 +40,7 @@ If you're on a smaller screen, scroll sideways to see whole table.
:::

<AddScript script-url="../../supported-devices.js"/>
<p>Search: <input type="text" class="deviceSearch"></p>
<p><input type="text" class="deviceSearch" placeholder="Enter Your Boardname Or Device Name"></p>
Copy link
Member

Choose a reason for hiding this comment

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

The field and text are a accessibility issue aswell due to missing contrast

@ninelore
Copy link
Member

Closing because author declared absence in Discord

@ninelore ninelore closed this Jul 11, 2024
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.

4 participants