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

Make thiner border for focused links #9277

Merged
merged 4 commits into from
Oct 17, 2022

Conversation

MRmlik12
Copy link
Contributor

Summary of the changes (in less than 80 characters):

  • Added support for Firefox browser
  • Make thinner border when navigating page with TAB or clicking a link

Addresses #9072

@MRmlik12 MRmlik12 requested a review from a team as a code owner October 11, 2022 14:00
@advay26
Copy link
Member

advay26 commented Oct 11, 2022

Thanks @MRmlik12! This looks really good. The new borders work for most things, but I noticed that it is now harder to see the borders around the Package Manager tabs (class="package-manager-tab"), especially the selected/active tab.

With the changes:
Contributor changes

Original:
Original

I think we should change the color of the border around the package manager tabs, picking something with a good contrast ratio (we target a contrast ratio greater than 3:1 for UI components, and something like #0078d4 seems to have a good contrast ratio with the package manager tabs' colors). We can target this to just these package manager tabs (class="package-manager-tab"), since all the other elements look great with the new changes.

@MRmlik12
Copy link
Contributor Author

color.mp4

@advay26 I changed the border width to 3px and used #0078d4 color. It's seems to be ok.

@advay26
Copy link
Member

advay26 commented Oct 11, 2022

I changed the border width to 3px and used #0078d4 color. It's seems to be ok.

Yep, this looks good to me. You can move forward with that change.

drewgillies
drewgillies previously approved these changes Oct 11, 2022
Copy link
Contributor

@drewgillies drewgillies 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. :) <3

}

@media (min-width: @screen-sm-min) {
li {
display: inline-block;
line-height: @package-list-line-height;
max-height: (@package-list-line-height * 5 + 1px);
overflow-y: hidden;
// overflow-y: hidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's delete the line rather than comment it.

@MRmlik12
Copy link
Contributor Author

Done

advay26
advay26 previously approved these changes Oct 12, 2022
}

@media (min-width: @screen-sm-min) {
li {
display: inline-block;
line-height: @package-list-line-height;
max-height: (@package-list-line-height * 5 + 1px);
overflow-y: hidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

@advay26 This seems enabling a broader change. Is it safe to do so?

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 removed this because the focus border was cutted on bottom

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what else this change might affect. @MRmlik12 Correct me if I'm wrong, but as far as I could tell, the 'overflow-y: hidden' was only affecting the border around the 'package tags' on the Search page right? We can limit this change to just those tags doing something like this:
image

This way we can keep the focus border around the tags intact and limit any unwanted side effects.

@@ -426,6 +431,10 @@
font-family: @font-family-base;
color: #323130;

&:focus {
outline-offset: 0;
Copy link
Member

Choose a reason for hiding this comment

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

We should also change the border color around the body tabs along with the package manager tabs
image

Suggested change
outline-offset: 0;
outline: 3px solid #0078D4;
outline-offset: 0;

@advay26 advay26 requested a review from ryuyu October 13, 2022 20:44
@MRmlik12
Copy link
Contributor Author

@advay26 @zhhyu Ok, I made some changes which you're suggested

Copy link
Member

@advay26 advay26 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, thanks @MRmlik12!

@advay26 advay26 added the Hacktoberfest Well-defined, self-contained issue open to community contribution label Oct 14, 2022
@advay26 advay26 merged commit a6706cb into NuGet:dev Oct 17, 2022
@advay26
Copy link
Member

advay26 commented Oct 17, 2022

Thanks for your contribution @MRmlik12! 😊

@MRmlik12 MRmlik12 deleted the Bug9072-thiner-border-links branch October 17, 2022 18:48
RiadGahlouz added a commit that referenced this pull request Oct 19, 2022
* Update NuGetGallery.Services to address CG alerts (#9274)

* Address accessibility for syntax highlighting (#9273)

* address accessbility for syntax highlighting

* Fix Support link (#9276)

* fix broken support link

* Update Download table heading (#9267)

* update correct place

* update table header placement

* Add instructions to install MSBuildSdk packages (#9268)

* Added "IsMSBuildSdkPackageType" to determine whether a package is of type MSBuildSdk.

DisplayPackage view modified to show specific instructions for SDK types in project files as per #8800

* Changed "Include" to correct attribute "Name" for SDK package type

Co-authored-by: Advay Tandon <[email protected]>
Co-authored-by: lyndaidaii <[email protected]>

* [CodeQL] Suppress CSRF token validation warnings (#9278)

* Added CSRF token checks to address CodeQL bugs

* Added CodeQL suppressions

* Make thinner border for focused links (#9277)

* Make thiner border for focused links

* Change border size of package manager tabs

* Delete comment line from base.less

* Change nav-tabs color and make overflow-y visible for package-tags

Co-authored-by: Joel Verhagen <[email protected]>
Co-authored-by: lyndaidaii <[email protected]>
Co-authored-by: toseni <[email protected]>
Co-authored-by: Ian Rathbone <[email protected]>
Co-authored-by: Advay Tandon <[email protected]>
Co-authored-by: Daniel Olczyk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest Well-defined, self-contained issue open to community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants