Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Provider): set scrollbar styles #1223

Merged
merged 6 commits into from
Apr 16, 2019
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Apr 15, 2019

This adds styles for scrollbar to Teams theme. I defined styles in ProviderBox instead of staticStyles to avoid collisions.

@codecov
Copy link

codecov bot commented Apr 15, 2019

Codecov Report

Merging #1223 into master will decrease coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1223      +/-   ##
==========================================
- Coverage   83.22%   83.21%   -0.01%     
==========================================
  Files         752      754       +2     
  Lines        8905     8912       +7     
  Branches     1194     1194              
==========================================
+ Hits         7411     7416       +5     
- Misses       1480     1482       +2     
  Partials       14       14
Impacted Files Coverage Δ
.../teams/components/Provider/providerBoxVariables.ts 100% <100%> (ø)
packages/react/src/themes/teams/componentStyles.ts 100% <100%> (ø) ⬆️
packages/react/src/lib/handleRef.ts 100% <100%> (ø) ⬆️
...kages/react/src/themes/teams/componentVariables.ts 100% <100%> (ø) ⬆️
...mes/teams/components/Provider/providerBoxStyles.ts 33.33% <33.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfd13c1...588da2c. Read the comment docs.

// `ref` is a mutable object
ref.current = node
// The `current` property is defined as readonly, however it's a valid way because `ref` is a mutable object
;(ref as React.MutableRefObject<N>).current = node
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ; cruft?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope and this is sad: prettier/prettier#4630 😭

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, just to mention I hate this, especially that we are not using ; at all... Sad day for me :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Even worse with the provided example, it is used in our guides... :\

scrollbarHeight: pxToRem(16),
scrollbarWidth: pxToRem(16),

scrollbarThumbBackgroundColor: siteVariables.gray06,
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, base theme doesn't have gray06 and gray04 :S

Copy link
Member Author

@layershifter layershifter Apr 15, 2019

Choose a reason for hiding this comment

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

Definitely, I initially overlooked the path of styles, now they are in correct place.

P.S. Motivation to add them only to Teams theme:

  • one of the frequent questions in SUI: "How I can disable custom scrollbars?"
  • this feature is not cross-browser

Copy link
Contributor

Choose a reason for hiding this comment

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

I would really love to see this as part of base theme, but I see your points. Let's think about this in the future. The base theme's provider should have custom scrollbar by default, at least in my opinion. Agreed that we should have way of enabling or disabling this behavior as well. The main reason for this is that, I don't see much different ways of defining the scrollbar, more over we can customize it trough it's variables.

Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

Changes look good, but are not covered by any test. Would it make sense to add a screener test for that?

@layershifter layershifter merged commit fe9cbf3 into master Apr 16, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/style-scrollbar branch April 16, 2019 15:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants