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 Button Shortcut Style Enhancements + Additional Storybook Tests #8947

Merged
merged 9 commits into from
Dec 10, 2024

Conversation

emshyu
Copy link
Contributor

@emshyu emshyu commented Dec 8, 2024

Resolves Issue #4871 (or enhances)

Additions

  • Customized Styling for Button Shortcuts: Previously, button shortcuts/separator styling was standardized. We added customized styling for optional button shortcuts based on button accents and variants.
  • Enhanced Storybook Catalogs: We modified ShortcutCatalog in Button.stories.tsx to display button shortcuts across styles and various inputs.

Screenshot 2024-12-07 at 9 16 35 PM
Screenshot 2024-12-07 at 9 16 30 PM
Screenshot 2024-12-07 at 9 16 22 PM

Copy link

github-actions bot commented Dec 8, 2024

Fails
🚫

node failed.

Log

�[31mError: �[39m RequestError [HttpError]: You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 4B47:ED6D4:2162D86:21F9922:6757ED8B.
    at /home/runner/work/twenty/twenty/node_modules/�[4m@octokit�[24m/request/dist-node/index.js:86:21
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m {
  status: �[33m403�[39m,
  response: {
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Aemshyu%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    status: �[33m403�[39m,
    headers: {
      �[32m'access-control-allow-origin'�[39m: �[32m'*'�[39m,
      �[32m'access-control-expose-headers'�[39m: �[32m'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'�[39m,
      connection: �[32m'close'�[39m,
      �[32m'content-encoding'�[39m: �[32m'gzip'�[39m,
      �[32m'content-security-policy'�[39m: �[32m"default-src 'none'"�[39m,
      �[32m'content-type'�[39m: �[32m'application/json; charset=utf-8'�[39m,
      date: �[32m'Tue, 10 Dec 2024 07:28:11 GMT'�[39m,
      �[32m'referrer-policy'�[39m: �[32m'origin-when-cross-origin, strict-origin-when-cross-origin'�[39m,
      server: �[32m'github.com'�[39m,
      �[32m'strict-transport-security'�[39m: �[32m'max-age=31536000; includeSubdomains; preload'�[39m,
      �[32m'transfer-encoding'�[39m: �[32m'chunked'�[39m,
      vary: �[32m'Accept-Encoding, Accept, X-Requested-With'�[39m,
      �[32m'x-content-type-options'�[39m: �[32m'nosniff'�[39m,
      �[32m'x-frame-options'�[39m: �[32m'deny'�[39m,
      �[32m'x-github-api-version-selected'�[39m: �[32m'2022-11-28'�[39m,
      �[32m'x-github-media-type'�[39m: �[32m'github.v3; format=json'�[39m,
      �[32m'x-github-request-id'�[39m: �[32m'4B47:ED6D4:2162D86:21F9922:6757ED8B'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1733815751'�[39m,
      �[32m'x-ratelimit-resource'�[39m: �[32m'search'�[39m,
      �[32m'x-ratelimit-used'�[39m: �[32m'1'�[39m,
      �[32m'x-xss-protection'�[39m: �[32m'0'�[39m
    },
    data: {
      documentation_url: �[32m'https://docs.github.com/free-pro-team@latest/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits'�[39m,
      message: �[32m'You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 4B47:ED6D4:2162D86:21F9922:6757ED8B.'�[39m
    }
  },
  request: {
    method: �[32m'GET'�[39m,
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Aemshyu%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    headers: {
      accept: �[32m'application/vnd.github.v3+json'�[39m,
      �[32m'user-agent'�[39m: �[32m'octokit-rest.js/18.12.0 octokit-core.js/3.6.0 Node.js/18.20.5 (linux; x64)'�[39m,
      authorization: �[32m'token [REDACTED]'�[39m
    },
    request: { hook: �[36m[Function: bound bound register]�[39m }
  }
}
danger-results://tmp/danger-results-e73a1cd3.json

Generated by 🚫 dangerJS against f4d7cf3

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Enhanced button shortcuts with customized styling based on variants and accents, while updating Yarn version and improving Storybook documentation.

  • Added variant-specific styling for shortcut separators and labels in packages/twenty-ui/src/input/button/components/Button.tsx with conditional colors based on button type
  • Enhanced ShortcutCatalog in Button stories to showcase shortcut styling across different button variants and accents
  • Upgraded Yarn package manager from 4.4.0 to 4.5.2 across configuration files
  • Removed commented code blocks in Button component implementation
  • Added proper color variations for shortcut text and vertical separators matching Figma design specs

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

6 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

@FelixMalfait
Copy link
Member

Screenshot 2024-12-08 at 18 42 24

Thanks @emshyu! I think the colors are different on Figma, also made a minor comment

https://twenty.com/developers/section/frontend-development/work-with-figma

@@ -416,8 +438,12 @@ export const Button = ({
{title}
{shortcut && (
<>
<StyledSeparator buttonSize={size} />
<StyledShortcutLabel>{shortcut}</StyledShortcutLabel>
{/* <StyledSeparator buttonSize={size} />*/}
Copy link
Member

Choose a reason for hiding this comment

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

Don't leave old code as comments, that's what we use git for, we can always find it later if we need to :)

package.json Outdated
@@ -343,7 +343,7 @@
},
"license": "AGPL-3.0",
"name": "twenty",
"packageManager": "yarn@4.4.0",
"packageManager": "yarn@4.5.2",
Copy link
Member

Choose a reason for hiding this comment

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

It might be a bit risky to change this as part of this unrelated PR but why not :) - anything in particular. that led you to change it? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I think it may have changed accidentally when I ran something with Yarn. This shouldn't be necessary.

@emshyu
Copy link
Contributor Author

emshyu commented Dec 8, 2024

I just added in the admin token

@emshyu
Copy link
Contributor Author

emshyu commented Dec 8, 2024

Screenshot 2024-12-08 at 18 42 24 Thanks @emshyu! I think the colors are different on Figma, also made a minor comment

https://twenty.com/developers/section/frontend-development/work-with-figma

Hi! I just updated the colors to match, should I make a new PR?

@emshyu
Copy link
Contributor Author

emshyu commented Dec 8, 2024

Screenshot 2024-12-08 at 18 42 24 Thanks @emshyu! I think the colors are different on Figma, also made a minor comment https://twenty.com/developers/section/frontend-development/work-with-figma

Hi! I just updated the colors to match, should I make a new PR?
Screenshot 2024-12-08 at 2 50 14 PM
Screenshot 2024-12-08 at 2 50 23 PM
Screenshot 2024-12-08 at 2 50 32 PM

@FelixMalfait FelixMalfait self-assigned this Dec 9, 2024
Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Thank you!

@FelixMalfait FelixMalfait merged commit 0654d81 into twentyhq:main Dec 10, 2024
14 checks passed
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.

2 participants