Improve contrast for icons in invisible button on hover in dark mode (

#2250) Co-authored-by: langermank <[email protected]> Co-authored-by: Cameron Dutro <[email protected]>
primer · Sep 26, 2023 · 66c4dd6 · 66c4dd6
1 parent fd17adf
commit 66c4dd6
Show file tree

Hide file tree

Showing 16 changed files with 62 additions and 23 deletions.
diff --git a/.changeset/green-buttons-happen.md b/.changeset/green-buttons-happen.md
@@ -0,0 +1,9 @@
+---
+"@primer/view-components": patch
+---
+
+- Improve contrast for icons in `invisible` button on hover in dark mode (within v8 colors)
+- Fix disabled button styles (v8 colors)
+- Bump Primitives to latest
+
+<!-- Changed components: Primer::Beta::Button -->
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -257,5 +257,8 @@ jobs:
           cache: 'npm'
       - name: NPM Build
         run: npm ci
+        env:
+          # Disable CSS minification for tests
+          CI: 'false'
       - name: Test CSS
         run: bundle exec rake test:component_css
diff --git a/...ts.test.ts-snapshots/primer/alpha/segmented_control/icon_only_large/focused.png b/...ts.test.ts-snapshots/primer/alpha/segmented_control/icon_only_large/focused.png
diff --git a/...ts.test.ts-snapshots/primer/alpha/segmented_control/icon_only_small/focused.png b/...ts.test.ts-snapshots/primer/alpha/segmented_control/icon_only_small/focused.png
diff --git a/...st.ts-snapshots/primer/alpha/segmented_control/icons_and_text_large/focused.png b/...st.ts-snapshots/primer/alpha/segmented_control/icons_and_text_large/focused.png
diff --git a/...s.test.ts-snapshots/primer/alpha/segmented_control/with_link_as_tag/focused.png b/...s.test.ts-snapshots/primer/alpha/segmented_control/with_link_as_tag/focused.png
diff --git a/...napshots.test.ts-snapshots/primer/beta/button/invisible_all_visuals/default.png b/...napshots.test.ts-snapshots/primer/beta/button/invisible_all_visuals/default.png
diff --git a/...shots/snapshots.test.ts-snapshots/primer/beta/button/leading_visual/focused.png b/...shots/snapshots.test.ts-snapshots/primer/beta/button/leading_visual/focused.png
diff --git a/Gemfile b/Gemfile
@@ -41,6 +41,8 @@ gem "bootsnap", ">= 1.4.2", require: false
 gem "lookbook", "~> 2.1.1" unless rails_version.to_f < 7
 gem "view_component", path: ENV["VIEW_COMPONENT_PATH"] if ENV["VIEW_COMPONENT_PATH"]
 
+gem "sourcemap"
+
 group :test do
   gem "webmock"
 

diff --git a/Gemfile.lock b/Gemfile.lock
@@ -195,6 +195,7 @@ GEM
     simplecov-html (0.12.3)
     simplecov_json_formatter (0.1.4)
     smart_properties (1.17.0)
+    sourcemap (0.1.1)
     sprockets (4.2.0)
       concurrent-ruby (~> 1.0)
       rack (>= 2.2.4, < 4)
@@ -265,6 +266,7 @@ DEPENDENCIES
   rubocop-performance (~> 1.7)
   simplecov (~> 0.21.2)
   simplecov-console (~> 0.9.1)
+  sourcemap
   sprockets
   sprockets-rails
   thor

diff --git a/app/components/primer/beta/button.pcss b/app/components/primer/beta/button.pcss
@@ -169,7 +169,7 @@ summary.Button {
   fill: var(--button-primary-iconColor-rest);
   background-color: var(--button-primary-bgColor-rest);
   border-color: var(--button-primary-borderColor-rest);
-  box-shadow: var(--shadow-resting-small, var(--color-btn-primary-shadow)), var(--shadow-highlight, var(--color-btn-primary-inset-shadow));
+  box-shadow: var(--shadow-resting-small, var(--color-btn-primary-shadow));
 
   &:hover:not(:disabled) {
     background-color: var(--button-primary-bgColor-hover);
@@ -247,8 +247,16 @@ summary.Button {
 .Button--invisible {
   color: var(--button-default-fgColor-rest);
 
+  &.Button--iconOnly {
+    color: var(--button-invisible-iconColor-rest, var(--color-fg-muted));
+  }
+
   &:hover:not(:disabled) {
     background-color: var(--button-invisible-bgColor-hover);
+
+    & .Button-visual {
+      color: var(--button-invisible-iconColor-hover, var(--color-fg-default));
+    }
   }
 
   &[aria-pressed='true'],
@@ -270,7 +278,7 @@ summary.Button {
   }
 
   & .Button-visual {
-    color: var(--fgColor-muted);
+    color: var(--button-invisible-iconColor-rest, var(--color-fg-muted));
 
     & .Counter {
       color: var(--fgColor-default);
@@ -318,7 +326,7 @@ summary.Button {
     fill: var(--button-danger-fgColor-hover);
     background-color: var(--button-danger-bgColor-hover);
     border-color: var(--button-danger-borderColor-hover);
-    box-shadow: var(--shadow-resting-small), var(--shadow-highlight);
+    box-shadow: var(--shadow-resting-small);
 
     & .Counter {
       color: var(--buttonCounter-danger-fgColor-hover);

diff --git a/demo/package-lock.json b/demo/package-lock.json
diff --git a/demo/package.json b/demo/package.json
@@ -4,7 +4,7 @@
   "version": "0.1.0",
   "dependencies": {
     "@primer/css": "^21.0.8",
-    "@primer/primitives": "7.12.0",
+    "@primer/primitives": "^7.14.0",
     "@rails/actioncable": "^7.0.7",
     "@rails/ujs": "^7.0.7",
     "turbolinks": "^5.2.0",

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -62,7 +62,7 @@
     "@github/prettier-config": "0.0.4",
     "@playwright/test": "^1.35.1",
     "@primer/css": "21.0.2",
-    "@primer/primitives": "7.12.0",
+    "@primer/primitives": "^7.14.0",
     "@primer/stylelint-config": "^12.7.2",
     "@rollup/plugin-node-resolve": "^13.3.0",
     "@rollup/plugin-typescript": "^8.3.3",

diff --git a/test/css/css_var_fallback_test.rb b/test/css/css_var_fallback_test.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "components/test_helper"
+require "sourcemap"
 
 class Primer::CssVariableTest < Minitest::Test
   class CssFile
@@ -14,6 +15,14 @@ def contents
       @contents ||= File.read(path)
     end
 
+    def sourcemap
+      @sourcemap ||= SourceMap::Map.from_json(File.read(sourcemap_path))
+    end
+
+    def sourcemap_path
+      @sourcemap_path ||= "#{path}.map"
+    end
+
     def find_offset(pos)
       line_idx = line_ranges.bsearch_index do |range|
         if pos < range.first
@@ -28,7 +37,13 @@ def find_offset(pos)
       return unless line_idx
 
       line_range = line_ranges[line_idx]
-      [line_idx + 1, pos - line_range.first]
+
+      offset = SourceMap::Offset.new(
+        line_idx + 1,
+        pos - line_range.first
+      )
+
+      sourcemap.bsearch(offset)
     end
 
     private
@@ -60,9 +75,9 @@ def test_css_rules_all_contain_fallbacks
     missing = [].tap do |results|
       css_file.contents.scan(regex) do
         start_pos, = Regexp.last_match.offset(0)
-        line, col = css_file.find_offset(start_pos)
-        source_file = Pathname(css_file.path).relative_path_from(Rails.root.join(".."))
-        results << "#{source_file}:#{line}:#{col}"
+        mapping = css_file.find_offset(start_pos)
+        source_file = File.join("app", *mapping.source.split(File::SEPARATOR)[2..])
+        results << "#{source_file}:#{mapping.original.line}:#{mapping.original.column}"
       end
     end