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 icon shrink and use avatar for logo and icons #9117

Conversation

ehconitin
Copy link
Contributor

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 PR fixes icon shrinking in tabs and standardizes logo/icon rendering using the Avatar component, while also addressing scroll wrapper issues affecting tab borders.

  • Added flex-shrink: 0 to StyledIconContainer in /packages/twenty-front/src/modules/ui/layout/tab/components/Tab.tsx to prevent icon compression
  • Replaced custom logo/icon implementations with unified Avatar component in Tab.tsx for consistent styling
  • Removed tabList scroll wrapper context to fix disappearing dark borders on active tabs
  • Added native overflow-y: scroll with hidden scrollbar to StyledTabsContainer in TabList.tsx

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

@charlesBochet
Copy link
Member

@ehconitin why do we remove the scrollwrapper in tab list? we want it in case we have too many tabs and need horizontal scrolling isn't it?

@ehconitin
Copy link
Contributor Author

I am not sure @charlesBochet, I thought this was discussed before merging #8726.
cc @ijreilly

@ehconitin
Copy link
Contributor Author

I reverted the tabList changes from #8726 and wrapped the tabList in WorkflowEditActionFormServerlessFunction in a container(as done elsewhere in the codebase).
Added scrollWrapper back similar to #9089!

@ijreilly
Copy link
Collaborator

ijreilly commented Dec 18, 2024

@charlesBochet the scrollwrapper had been removed in a PR that was merged before our refacto, but that we did not see because i had not pulled main recently enough
https://github.com/twentyhq/twenty/pull/9016/files#diff-914c8b53acb31383ce3b590b56488ada3db3d4666be54632a907b2f9b447de20
image

@ehconitin reached out this morning since our refacto was causing an issue on tablist. It was ok for me to remove our scrollwrapper as it had been removed already before

@charlesBochet
Copy link
Member

That's a mistake, we still need the scrollWrapper!

@ehconitin
Copy link
Contributor Author

@charlesBochet added it back! ready for review!

@charlesBochet
Copy link
Member

Discussed this one with @Bonapara to understand the change:

  • The current Avatar component that is able to take a logo, an icon, should be renamed to Icon
  • we should introduce an other sub-component called IconAvatar that should only take logo or placeholder into props
  • current Icon should become IconTablerIcon
  • we will introduce other types of icons later such as: IconColor, IconFilledIcon, IconEmoji

@charlesBochet charlesBochet merged commit 5e03f4d into twentyhq:main Dec 18, 2024
20 checks passed
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 5417:2EF790:61BB19F:C1B9C35:67630F44.
    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%3Aehconitin%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'Wed, 18 Dec 2024 18:07:00 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'5417:2EF790:61BB19F:C1B9C35:67630F44'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1734545280'�[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 5417:2EF790:61BB19F:C1B9C35:67630F44.'�[39m
    }
  },
  request: {
    method: �[32m'GET'�[39m,
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Aehconitin%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-0bcb2e3f.json

Generated by 🚫 dangerJS against 319f276

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

Successfully merging this pull request may close these issues.

3 participants