Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: Add LicenseBanner#3568

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
presleyp merged 22 commits intomainfromlicense-banner/presleyp/3217
Aug 23, 2022
Merged

feat: Add LicenseBanner#3568

presleyp merged 22 commits intomainfromlicense-banner/presleyp/3217
Aug 23, 2022

Conversation

presleyp
Copy link
Contributor

@presleyppresleyp commentedAug 18, 2022
edited
Loading

Fixes#3217

Showing the color scheme (more up to date than the gif below):
Screen Shot 2022-08-18 at 6 59 17 PM

Description

  • ExtractsPill component fromWorkspaceStatusBadge to reuse inLicenseBannerView
  • ExtractsExpander component fromErrorSummary to reuse inLicenseBannerView
  • implements license banner design, sort of, inLicenseBannerView - see remaining issues below!
  • hooks upLicenseBanner toentitlementsXService.

Left to do:

  • improve the colors
  • see where I need more stories and tests

To try it out

The current api will always return falsey data, as if you are using the open source offering. To view the banner, use the XState inspector to send an event of type "SHOW_MOCK_BANNER". To hide the banner, send, you guessed it, "HIDE_MOCK_BANNER."

Kapture 2022-08-18 at 17 16 29

Design changes

Here is theFigma. I have deviated from it in some ways:

  • The banner is above the nav bar, like in v1. Ammar agreed this made the most sense since it's a global issue.
  • The expand/collapse control is a link with an icon rather than embedded text. This is for consistency withErrorSummary.
  • The color scheme is orange, because people agreed it should be warning-colored.
  • The pill says "License Issue" or "n License Issues" for consistency (rather than just "License" in the singular case) and the casing is not all caps for consistency withWorkspaceStatusBadge

I also changed the color of the More/Less link from light blue to near-white, which affected theErrorSummary. Let me know if you think it's a problem!

kylecarbs reacted with hooray emoji
@presleyppresleyp changed the titleLicense banner/presleyp/3217feat: Add LicenseBannerAug 18, 2022
@presleyppresleyp marked this pull request as ready for reviewAugust 19, 2022 18:16
@presleyppresleyp requested a review froma team as acode ownerAugust 19, 2022 18:16
@presleyppresleyp requested review fromBrunoQuaresma andKira-Pilot and removed request fora teamAugust 19, 2022 18:16
const styles = useStyles()
interface ArrowProps {
margin?: boolean
}
Copy link
Member

Choose a reason for hiding this comment

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

We could share this typing with themakeStyles block above:makeStyles<Theme, ArrowProps>

Copy link
Member

Choose a reason for hiding this comment

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

I also wonder if, instead of amargin prop, we add astyle prop that we spread in the children. Makes things a bit more flexible, but just a suggestion!

Copy link
ContributorAuthor

@presleyppresleypAug 22, 2022
edited
Loading

Choose a reason for hiding this comment

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

We may want to consider keepinglink text blue for accessibility reasons.

(This comment ended up in the wrong place, sorry for the confusion!)

@Kira-Pilot Yeah, so I changed this because I thought the color combination was bad and it was also going to pose a contrast issue, but I have been wondering if it's obvious enough. I think the chevron and placement helps. But I also think I'm having trouble making it blue or underlined because it's a not a navigation link. It's really a button in functionality. What do you think is the best way to style that? I guess Zenhub styles "Show 4 more" on this page like a link (bold and blue).

Kira-Pilot reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Going to wait on astyle prop here because I think this component will be revisited in future color work and it'll be easier to tell then what to do.

Kira-Pilot reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

@presleyp That makes sense! I agree with your logic and I don't think it's something we need to change right now. Let's keep it back of mind and look out for link inspo in the future.

presleyp reacted with thumbs up emoji
presleypand others added2 commitsAugust 22, 2022 10:25
Co-authored-by: Kira Pilot <kira@coder.com>
Co-authored-by: Kira Pilot <kira@coder.com>
@Kira-Pilot
Copy link
Member

We may want to consider keepinglink text blue for accessibility reasons. No strong opinions on this thought - I think we still have a lot of contrast.

This looks great! Really thorough and nice job.

@presleyppresleyp merged commit49de44c intomainAug 23, 2022
@presleyppresleyp deleted the license-banner/presleyp/3217 branchAugust 23, 2022 15:26
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@Kira-PilotKira-PilotKira-Pilot approved these changes

@BrunoQuaresmaBrunoQuaresmaAwaiting requested review from BrunoQuaresmaBrunoQuaresma was automatically assigned from coder/ts

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show license banner when warnings are present
2 participants
@presleyp@Kira-Pilot

[8]ページ先頭

©2009-2025 Movatter.jp