Skip to content

Remove popover responsive hack - #1759

Merged
langermank merged 7 commits into
mainfrom
hx-popover-remedy
Nov 22, 2021
Merged

Remove popover responsive hack#1759
langermank merged 7 commits into
mainfrom
hx-popover-remedy

Conversation

@langermank

@langermanklangermank commented Nov 19, 2021

Copy link
Copy Markdown
Contributor
  • Copies Popoverhack into Primer CSS, removing .page-responsive
  • By default, on screens smaller than 767pxPopover will be full-width and without caret
  • Removed dependency on Box
  • Organized docs a bit

Fixes: https://github.com/github/primer/issues/324

/cc @primer/css-reviewers

@langermank
langermank requested a review from a team as a code ownerNovember 19, 2021 19:17
@changeset-bot

changeset-botBot commented Nov 19, 2021•
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ac124d2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/cssMinor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jonrohan
jonrohan approved these changes Nov 22, 2021
View reviewed changes

@jonrohanjonrohan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, and Awesome that you're adding stories as you're going ✨

@langermank
langermank merged commit 74724ce into mainNov 22, 2021
@langermank
langermank deleted the hx-popover-remedy branch November 22, 2021 19:23
@primer-cssprimer-css mentioned this pull request Nov 22, 2021
Merged
simurai
simurai reviewed Nov 23, 2021
View reviewed changes
Comment threadsrc/popover/popover.scss
Comment on lines +202 to +205
top: auto;
right: 0;
bottom: 0;
left: 0;

@simuraisimuraiNov 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@langermank Sorry this is too late and would need to be a follow-up PR.

But we might wanna keep the !important. There are cases where Popovers are positioned manually to adjust to certain content. E.g. using right: -327px; bottom: -8px.

Also the width of the Popover-message that probably gets customized too. E.g. width: 250px;

@langermanklangermankNov 23, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah! No problem, will follow up with a fix. Thanks :)

@simuraisimurai mentioned this pull request Nov 25, 2021
Merged
@primer-cssprimer-css mentioned this pull request Dec 6, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simuraisimuraisimurai left review comments
@jonrohanjonrohanjonrohan approved these changes
@pouretrebellepouretrebelleAwaiting requested review from pouretrebellepouretrebelle was automatically assigned from primer/css-reviewers

Assignees

No one assigned

Labels

minor release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@langermank@jonrohan@simurai