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

fix #7781 made kanban board title and checkbox 24px #7815

Merged

Conversation

NitinPSingh
Copy link
Contributor

issue: #7781

  • titlechip to 24px
  • checkbox to 24px
    Screenshot 2024-10-18 134759
    Screenshot 2024-10-18 134708

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

This pull request addresses issue #7781 by adjusting the sizes of the Checkbox and Chip components to 24px height.

  • Modified Checkbox.tsx to set small size variant to 24px (previously 22px)
  • Updated Chip.tsx to increase height from 20px to 24px using theme.spacing(4)
  • Aligned Chip component with Figma design specifications for 24px height and medium weight
  • Ensured Checkbox container matches the 24px height requirement as per Figma design
  • Changes affect visual consistency of Kanban Card header components

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

Comment on lines 130 to 131
--size: ${({ checkboxSize }) =>
checkboxSize === CheckboxSize.Large ? '16px' : '12px'};
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Icon size for small checkbox remains 12px. Consider increasing to 14px for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i didn't made it 12px but changing it to 14px for consistency

@Bonapara
Copy link
Member

Hi @NitinPSingh, thanks for the PR. Please ensure that only the padding or icon container changes, not the icon size or width. For example, the checkbox size changed from 14px to 16px in list views.

CleanShot.2024-10-18.at.12.13.30.mp4

Also, I'm not sure I understand what Greptile suggested. Here is the Checkboxes Figma Component. Please review it to ensure the components reflect the design.

image

https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=14603-51328&node-type=frame&t=OOVK3xhrcy5zJPVL-11

Thanks for contributing, @NitinPSingh!

@Bonapara
Copy link
Member

@lucasbordeau lgtm from a front-end point of view but does spacing 1.25/1.5 is accepted?

@lucasbordeau
Copy link
Contributor

@Bonapara Sure ! Sometimes we have to obtain 1px precision.

Copy link
Member

@Bonapara Bonapara left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Nitin!

@lucasbordeau lucasbordeau merged commit e50117e into twentyhq:main Oct 18, 2024
13 checks passed
Copy link

oss-gg bot commented Oct 18, 2024

Awarding NitinPSingh: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/NitinPSingh

Copy link

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 9C27:257DD2:1631C14:16616CE:671286DD.
    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%3ANitinPSingh%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'Fri, 18 Oct 2024 16:03:41 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'9C27:257DD2:1631C14:16616CE:671286DD'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1729267481'�[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 9C27:257DD2:1631C14:16616CE:671286DD.'�[39m
    }
  },
  request: {
    method: �[32m'GET'�[39m,
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3ANitinPSingh%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.4 (linux; x64)'�[39m,
      authorization: �[32m'token [REDACTED]'�[39m
    },
    request: { hook: �[36m[Function: bound bound register]�[39m }
  }
}
danger-results://tmp/danger-results-0f47fed6.json

Generated by 🚫 dangerJS against f6f9549

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.

3 participants