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

Favorites Folders Fast Followups #8920

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

ehconitin
Copy link
Contributor

@ehconitin ehconitin commented Dec 6, 2024

Fixes :
Screenshot 2024-12-06 at 3 45 41 PM

Fixes: Reduce menu width to 160px and it should appear below three dots
Screenshot 2024-12-06 at 3 46 49 PM

Fixes: The right margin should be 2px
Screenshot 2024-12-06 at 3 47 45 PM

Fixes:
Requirement: Clicking the heart Icon should put the record as favorite. It shouldn't open the menu on first click. It should only on second click, when the record is already a favorite.

@ehconitin ehconitin marked this pull request as draft December 6, 2024 10:14
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 refines the favorites folder functionality with UI and interaction improvements across navigation drawer and dropdown components.

  • Changed FavoriteFolderNavigationDrawerItemDropdown dropdown placement to 'bottom-start' with fixed 160px width for better positioning
  • Added StyledOrphanFavoritesContainer in CurrentWorkspaceMemberFavoritesFolders for consistent spacing between favorite items
  • Implemented handleAddToFavorites in PageFavoriteFolderDropdown to prevent duplicate favorites
  • Reduced right padding from 1 to 0.5 spacing units in NavigationDrawerItem for better layout with folder options
  • Simplified NavigationDrawerSectionTitle by moving click handler to StyledLabel and removing redundant right icon interactions

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

@ehconitin ehconitin marked this pull request as ready for review December 6, 2024 14:54
@ehconitin ehconitin requested a review from martmull December 6, 2024 14:54
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

(updates since last review)

This PR updates the favorites folder UI behavior to ensure the dropdown only appears after an item is favorited, along with several UI refinements.

  • Modified RecordShowPageBaseHeader to conditionally render PageFavoriteFoldersDropdown only when item is favorited
  • Removed duplicate click handlers in NavigationDrawerSectionTitle to prevent double-firing
  • Simplified PageFavoriteFolderDropdown by removing useCreateFavorite hook and unused imports
  • Added margin fixes and menu width constraints for better dropdown positioning

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

@@ -77,7 +77,7 @@ const StyledItem = styled('button', {

padding-bottom: ${({ theme }) => theme.spacing(1)};
padding-left: ${({ theme }) => theme.spacing(1)};
padding-right: ${({ theme }) => theme.spacing(1)};
padding-right: ${({ theme }) => theme.spacing(0.5)};
Copy link
Member

Choose a reason for hiding this comment

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

@ehconitin this will impact all navigation drawer items, is this expected? (not only the workspace favorites). This seems suspicious!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok but how is it now broken on main branch ? it seems fine to me at least on production?

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 am not sure! I went through history of NavigationDrawerItem and it was always like this i.e, 4px padding all around

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charlesBochet should we close this? or do you need me to dig deeper into it?

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Thanks @ehconitin, left comments!

I have tested if functionnally and it feels nicer indeed

@@ -17,7 +17,10 @@ const StyledTitle = styled.div`
font-size: ${({ theme }) => theme.font.size.xs};
font-weight: ${({ theme }) => theme.font.weight.semiBold};
height: ${({ theme }) => theme.spacing(5)};
padding: ${({ theme }) => theme.spacing(1)};
padding-left: ${({ theme }) => theme.spacing(1)};
padding-right: ${({ theme }) => theme.spacing(0.5)};
Copy link
Member

Choose a reason for hiding this comment

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

this padding right 0.5 is suspicious!

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

LGTM

@charlesBochet charlesBochet merged commit c638429 into twentyhq:main Dec 17, 2024
17 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 9447:3AC688:2F4B4A:5E33A0:67615D90.
    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'Tue, 17 Dec 2024 11:16:32 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'9447:3AC688:2F4B4A:5E33A0:67615D90'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1734434252'�[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 9447:3AC688:2F4B4A:5E33A0:67615D90.'�[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-afcf4d47.json

Generated by 🚫 dangerJS against 2993daf

@ehconitin ehconitin deleted the FavoriteFoldersFastFollowups branch December 28, 2024 13:26
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.

2 participants