Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a proof of concept for the following idea: Because we have a few patterns/components that are quite similar and they all need to become responsive, why not combine them all with some sort of container object. Make it reusable for any kind of content.
We do this by:
.Overlay
object.SelectMenu
componentWhich lets us potentially deprecate:
.select-menu
.dropdown-menu
.Box-overlay
.Popover
Example
Below a simple example, more in the documentation.
.Overlay--dropdown
.Overlay--selectMenu
.Overlay--popover
☝️
< sm
breakpoint.Benefits
.Overlay
object for multiple patterns, we can greatly reduce CSS..Overlay
object, we can keep the look consistent.Concerns
It might be harder to maintain because changes to
.Overlay
need to be tested with many use-cases in mind.The "API" is not as intuitive. Currently it uses multiple modifier classes of the components it tries to replace. E.g.
.Overlay--selectMenu
. But it shouldn't describe the content, it should only describe the different "versions" of the.Overlay
object. Some alternatives:.Overlay--dropdown
->.Overlay--min
,.Overlay--small
.Overlay--selectMenu
->.Overlay--max
,.Overlay--large
,.Overlay-xs-fullHeight
.Overlay--popover
->.Overlay--transparent
Not matter how often I renamed them, it just doesn't feel right.
Having a lot of flexibility is great, but it might come at a cost of not being consistent throughout many use cases.
Alternatives
An alternative to this PR: Keep the components separated, maybe refactor/rename them and make them responsive. See https://github.com/github/design-systems/issues/601#issuecomment-488263465. Overall more CSS would be needed and the same/similar styling is repeated in multiple components. But at the same time they should be easier to maintain since they don't try to be too many things at once.
Ref. https://github.com/github/design-systems/issues/601#issuecomment-489551559