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

Limits changed to Elevated Access, margin added + copy changed #1010

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Elson9
Copy link
Contributor

@Elson9 Elson9 commented Mar 8, 2024

image

Margin added to prevent two-tiered access from looking like a separate product.
Removed "has a rate limit enforced" as this is not a strict requirement of two-tiered access.
Limits changed to Elevated Access to avoid confusion.
Added text to clarify that public access does not require an API key.


🚀 Feature branch deployment: https://api-services-portal-feature-directory-changes.apps.silver.devops.gov.bc.ca

@rustyjux
Copy link
Contributor

Design wise I think it's a step in the right direction towards clarifying that elevated access is not a separate product.

Additional changes to consider:

  • Reduce top margin (above Elevated Access).
  • "Public access does not require..." could be included directly under the public product name which it relates to.
  • Consider removing "For elevated access, please request access" entirely, since it should be more apparent what the "Request Access" button relates to.

@Elson9
Copy link
Contributor Author

Elson9 commented Mar 13, 2024

Add in placeholder button

@Elson9
Copy link
Contributor Author

Elson9 commented Mar 13, 2024

Latest changes:
image

@rustyjux
Copy link
Contributor

rustyjux commented Mar 13, 2024

Hey nice. Three thoughts:

  • Instead of disabled could we see the secondary style? (to compare at least) (variant="secondary")
    image
  • Further reduce padding/margins - there is 40 px between the public/elevated buttons right now, while there is only 30 px between the buttons and the dividers beyond. I feel like these should be same value, or if anything the space between the associated items should actually be less.
  • Use sentence case 'No API key required' - I think the title case (Request Access) is appropriate when we have buttons with short names but otherwise sentence case is consistent with elsewhere (e.g. "View metrics in real-time") and looks better (IMO)

@Elson9
Copy link
Contributor Author

Elson9 commented Mar 14, 2024

Thanks for your suggestions. Made those changes:
image

<AccessRequestForm
disabled={false}
id={id}
name={data.name}
preview={preview}
/>
) : (
<Button
disabled={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
disabled={true}

Copy link
Contributor

Choose a reason for hiding this comment

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

or rather ={false} if we keep that attribute around usually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants