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

[PWA-156] Added Drawer Footer Styleguide. #2093

Merged
merged 32 commits into from
Jan 27, 2020

Conversation

revanth0212
Copy link
Contributor

@revanth0212 revanth0212 commented Jan 10, 2020

Description

Drawer Footer Styleguide.

Related Issue

Closes PWA-156

Verification Stakeholders

@jimbo @soumya-ashok @jcalcaben

Verification Steps

  1. yarn workspace @magento/venia-styleguide run start
  2. Navigate to /page/drawer-footer

Screenshots

image

Checklist

  1. To be merged only after Add a button section to the styleguide #2088 is merged.

@revanth0212 revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jan 10, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jan 10, 2020

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-156.

Generated by 🚫 dangerJS against 0a881f0

jimbo
jimbo previously approved these changes Jan 14, 2020
@jimbo
Copy link
Contributor

jimbo commented Jan 14, 2020

Approved, but blocked by preceding PRs:

@revanth0212 revanth0212 dismissed jimbo’s stale review January 17, 2020 19:25

The base branch was changed.

@revanth0212 revanth0212 changed the base branch from jimbo/styleguide-buttons to develop January 17, 2020 19:25
@soumya-ashok
Copy link

soumya-ashok commented Jan 22, 2020

@revanth0212 @jimbo I've just had a look at the implementation.

A few questions:

  • I see a white background behind the buttons. Is that intentional?
  • Also, I assume that the way they are placed is meant to indicate that they should be vertically and horizontally aligned to or around the center, depending on how many buttons there are but this is not obvious.
  • In the first two button example, we have 2 secondary buttons. This shouldn't typically happen. The more regular case would be a secondary and primary like in the third example.
  • Are we addressing break-points and relative spacing or is this for the smallest mobile view?

@revanth0212
Copy link
Contributor Author

I see a white background behind the buttons. Is that intentional?

As far as I remember, this is intentional. I can revert it if that's not expected.

Also, I assume that the way they are placed is meant to indicate that they should be vertically and horizontally aligned to or around the center, depending on how many buttons there are but this is not obvious.

If we need a vertical example, I can add it to the styleguide. @jimbo what do you think?

In the first two button example, we have 2 secondary buttons. This shouldn't typically happen. The more regular case would be a secondary and primary like in the third example.

Can it not be a case where 2 buttons will definitely not be primary? As an example, next and back both can be secondary.

Are we addressing break-points and relative spacing or is this for the smallest mobile view?

Not sure of this.

@jimbo
Copy link
Contributor

jimbo commented Jan 23, 2020

I see a white background behind the buttons. Is that intentional?

Yes. Drawers (and footers) have a white background.

Also, I assume that the way they are placed is meant to indicate that they should be vertically and horizontally aligned to or around the center, depending on how many buttons there are but this is not obvious.

Yes, there are a maximum of two buttons, vertically and horizontally centered in a single row. There is not an example of vertically stacked buttons in a footer, because a footer has a single row.

In the first two button example, we have 2 secondary buttons. This shouldn't typically happen. The more regular case would be a secondary and primary like in the third example.

@soumya-ashok Agreed, examples should show one high and one low priority button, since that's the typical case.

@revanth0212 You're correct, it could potentially be two low priority buttons, but that would be unusual.

Are we addressing break-points and relative spacing or is this for the smallest mobile view?

In this case, there are no breakpoints, since a drawer's width (and thus the footer's width) matches our smallest breakpoint.

@soumya-ashok
Copy link

@revanth0212 and @jimbo Thanks for clarifying.

We don't need vertically stacked buttons, I meant center-aligned along the height.
There could be a case when someone chooses to use 2 secondary buttons alongside one another but I don't want to advocate it. Even in the case of "next" and "back", "next" is presumably the preferred action, and would be indicated with a primary button.

@revanth0212
Copy link
Contributor Author

revanth0212 commented Jan 27, 2020

There could be a case when someone chooses to use 2 secondary buttons alongside one another but I don't want to advocate it. Even in the case of "next" and "back", "next" is presumably the preferred action, and would be indicated with a primary button.

You are right here. But consider this scenario, the Drawer Footer is just 1 component in a page and sometimes it may not have a primary action but the page housing the drawer footer might have. In such situations, having 2 secondary buttons it is a valid use case.

@revanth0212
Copy link
Contributor Author

We can merge this PR for now and circle back later to discuss the missing use cases once we have a matured style guide.

@dpatil-magento dpatil-magento merged commit 5542122 into develop Jan 27, 2020
@dpatil-magento dpatil-magento deleted the revanth/footer-styleguide branch January 27, 2020 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:peregrine pkg:venia-styleguide version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants