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

Added firefox specific css. #2233

Merged
merged 4 commits into from
Mar 11, 2020
Merged

Conversation

revanth0212
Copy link
Contributor

@revanth0212 revanth0212 commented Mar 10, 2020

Description

Accordion text is not center-aligned in firefox. This is probably because of the way firefox interprets height on div. Giving line-height for divs in firefox fixes the problem.

Related Issue

Closes PWA-384

Verification Stakeholders

@jimbo

Verification Steps

  1. Go to cart in firefox.
  2. Text in accordions should be vertically center aligned.

Screenshots / Screen Captures (if appropriate)

image

image

Checklist

  • No functionality should have changed. This is just a css change.
  • Change should not effect css in chrome and safari. This is specific to firefox.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Mar 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-384.

Generated by 🚫 dangerJS against 6a26896

@revanth0212 revanth0212 added the version: Patch This changeset includes backwards compatible bug fixes. label Mar 10, 2020
@revanth0212 revanth0212 marked this pull request as ready for review March 10, 2020 19:12
Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

@revanth0212 Thanks for picking this one up; I apologize, I thought it had been assigned to me.

Unfortunately, this doesn't address the root issue, which is that the implementation is relying on display: flex working on a replaced element (<button> in this case). Also, we should avoid tricks that attempt to target Firefox specifically.

There are two things to fix with the accordion button here:

  • <button> is not a reliable flex container or grid container
  • <div> is not a valid child of <button>

So we need to change the <div> to a <span> and make the <span> the grid container for the whole element. The <button> should not contribute much to the layout; it's there purely for accessibility reasons.

@m2-community-project m2-community-project bot moved this from Ready for Review to Changes Requested in Pull Request Progress Mar 10, 2020
jimbo
jimbo previously approved these changes Mar 10, 2020
@supernova-at
Copy link
Contributor

✅ Verification steps pass

@dpatil-magento dpatil-magento merged commit dd40938 into develop Mar 11, 2020
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Mar 11, 2020
@dpatil-magento dpatil-magento deleted the revanth/firefox_accordion_css_fix branch March 11, 2020 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:venia-ui version: Patch This changeset includes backwards compatible bug fixes.
Development

Successfully merging this pull request may close these issues.

None yet

5 participants