Add beta layouts #937
Closed
Add beta layouts #937
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/view-components/BcDPZiX9caxvgMWYUnvx2RL9DtMy |
Closed
jklina
commented
Dec 7, 2021
Sync with upstream Primer / CSS changes
colebemis
reviewed
Jan 24, 2022
There was a problem hiding this comment.
Left a few suggestions to align the PageLayout ViewComponent API to the proposed React API. Think of these comments as conversation starters, not strong opinions 😄
Comment on lines
+22
to
+29
| WRAPPER_SIZING_DEFAULT = :fluid | ||
| WRAPPER_SIZING_MAPPINGS = { | ||
| WRAPPER_SIZING_DEFAULT => "", | ||
| :md => "container-md", | ||
| :lg => "container-lg", | ||
| :xl => "container-xl" | ||
| }.freeze | ||
| WRAPPER_SIZING_OPTIONS = WRAPPER_SIZING_MAPPINGS.keys.freeze |
There was a problem hiding this comment.
Suggested change
| WRAPPER_SIZING_DEFAULT = :fluid | |
| WRAPPER_SIZING_MAPPINGS = { | |
| WRAPPER_SIZING_DEFAULT => "", | |
| :md => "container-md", | |
| :lg => "container-lg", | |
| :xl => "container-xl" | |
| }.freeze | |
| WRAPPER_SIZING_OPTIONS = WRAPPER_SIZING_MAPPINGS.keys.freeze | |
| CONTAINER_WIDTH_DEFAULT = :full | |
| CONTAINER_WIDTH_MAPPINGS = { | |
| WRAPPER_SIZING_DEFAULT => "", | |
| :medium => "container-md", | |
| :large => "container-lg", | |
| :xlarge => "container-xl" | |
| }.freeze | |
| CONTAINER_WIDTH_OPTIONS = CONTAINER_WIDTH_MAPPINGS.keys.freeze |
| # @param primary_region [Symbol] When `responsive_variant` is set to `:separate_regions`, defines which region appears first on small viewports. `:content` is default. | ||
| # @param system_arguments [Hash] <%= link_to_system_arguments_docs %> | ||
| def initialize( | ||
| wrapper_sizing: WRAPPER_SIZING_DEFAULT, |
There was a problem hiding this comment.
Suggested change
| wrapper_sizing: WRAPPER_SIZING_DEFAULT, | |
| container_width: CONTAINER_WIDTH_DEFAULT, |
| class Content < Primer::Component | ||
| status :beta | ||
|
|
||
| WIDTH_DEFAULT = :fluid |
There was a problem hiding this comment.
Suggested change
| WIDTH_DEFAULT = :fluid | |
| WIDTH_DEFAULT = :full |
| status :beta | ||
|
|
||
| WIDTH_DEFAULT = :fluid | ||
| WIDTH_OPTIONS = [WIDTH_DEFAULT, :md, :lg, :xl].freeze |
There was a problem hiding this comment.
Suggested change
| WIDTH_OPTIONS = [WIDTH_DEFAULT, :md, :lg, :xl].freeze | |
| WIDTH_OPTIONS = [WIDTH_DEFAULT, :medium, :large, :xlarge].freeze |
|
|
||
| def call | ||
| render(Primer::BaseComponent.new(**@system_arguments)) do | ||
| if @width == :fluid |
There was a problem hiding this comment.
Suggested change
| if @width == :fluid | |
| if @width == :full |
| # <% c.footer_region(divider: true, border: true) { "Header" } %> | ||
| # <% end %> | ||
| # | ||
| # @param wrapper_sizing [Symbol] Define the maximum width of the component. `:fluid` sets it to full-width. Other values center Layout horizontally. <%= one_of(Primer::Beta::PageLayout::WRAPPER_SIZING_OPTIONS) %> |
There was a problem hiding this comment.
Suggested change
| # @param wrapper_sizing [Symbol] Define the maximum width of the component. `:fluid` sets it to full-width. Other values center Layout horizontally. <%= one_of(Primer::Beta::PageLayout::WRAPPER_SIZING_OPTIONS) %> | |
| # @param container_size [Symbol] Define the maximum width of the page container. `:full` sets it to full-width. Other values center Layout horizontally. <%= one_of(Primer::Beta::PageLayout::CONTAINER_WIDTH_OPTIONS) %> |
| # @param width [Symbol] <%= one_of(Primer::Beta::PageLayout::Content::WIDTH_OPTIONS) %> | ||
| # @param tag [Symbol] <%= one_of(Primer::Beta::PageLayout::Content::TAG_OPTIONS) %> | ||
| # @param system_arguments [Hash] <%= link_to_system_arguments_docs %> | ||
| renders_one :content_region, "Primer::Beta::PageLayout::Content" |
There was a problem hiding this comment.
Suggested change
| renders_one :content_region, "Primer::Beta::PageLayout::Content" | |
| renders_one :content, "Primer::Beta::PageLayout::Content" |
| # @param divider [Boolean] Whether to show a header divider | ||
| # @param divider_narrow [Symbol] Whether to show a divider below the `header` region if in responsive mode. <%= one_of(Primer::Beta::PageLayout::HEADER_DIVIDER_NARROW_OPTIONS) %> | ||
| # @param system_arguments [Hash] <%= link_to_system_arguments_docs %> | ||
| renders_one :header_region, lambda { |divider: false, divider_narrow: :line, **header_system_arguments| |
There was a problem hiding this comment.
Suggested change
| renders_one :header_region, lambda { |divider: false, divider_narrow: :line, **header_system_arguments| | |
| renders_one :header, lambda { |divider: false, divider_narrow: :line, **header_system_arguments| |
| # @param divider [Boolean] Whether to show a footer divider | ||
| # @param divider_narrow [Symbol] Whether to show a divider below the `footer` region if in responsive mode. <%= one_of(Primer::Beta::PageLayout::FOOTER_DIVIDER_NARROW_OPTIONS) %> | ||
| # @param system_arguments [Hash] <%= link_to_system_arguments_docs %> | ||
| renders_one :footer_region, lambda { |divider: false, divider_narrow: FOOTER_DIVIDER_NARROW_DEFAULT, **footer_system_arguments| |
There was a problem hiding this comment.
Suggested change
| renders_one :footer_region, lambda { |divider: false, divider_narrow: FOOTER_DIVIDER_NARROW_DEFAULT, **footer_system_arguments| | |
| renders_one :footer, lambda { |divider: false, divider_narrow: FOOTER_DIVIDER_NARROW_DEFAULT, **footer_system_arguments| |
| # @param position_narrow [Symbol] Pane placement when `Layout` is in column modes. <%= one_of(Primer::Beta::PageLayout::Pane::POSITION_NARROW_OPTIONS) %> | ||
| # @param divider [Boolean] Whether to show a pane line divider. | ||
| # @param system_arguments [Hash] <%= link_to_system_arguments_docs %> | ||
| renders_one :pane_region, lambda { | |
There was a problem hiding this comment.
Suggested change
| renders_one :pane_region, lambda { | | |
| renders_one :pane, lambda { | |
|
Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 adds the beta
SplitPageLayoutandPageLayoutcomponents which will replace the existing alphaLayoutcomponent.This cannot be deployed until the corresponding CSS is first deployed: primer/css#1737 and primer/css#1849