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

Kanban card creation followup #7285

Merged
merged 6 commits into from
Oct 3, 2024

Conversation

ehconitin
Copy link
Contributor

@ehconitin ehconitin commented Sep 27, 2024

@Bonapara #7002
@FelixMalfait #6316
;)

Naming could be improved, do let me know!

2024-09-27.15-13-06.mp4

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 implements improvements to the Kanban card creation process, addressing issues related to regular Kanban boards and Opportunity Kanban boards.

  • Added conditional logic in RecordBoardColumnCardsContainer.tsx to handle different card creation scenarios based on object type and field settings
  • Implemented company selection for Opportunities in RecordBoardColumnHeader.tsx when the company field is not disabled
  • Created new state management for Opportunities using Recoil in recordBoardNewOpportunityByColumnIdComponentFamilyState.ts and recordBoardNewOpportunityByColumnIdSelector.ts
  • Refactored useAddNewOpportunity.ts to support more flexible positioning options and improve modularity
  • Updated RecordIndexPageKanbanAddButton.tsx to simplify the add button component and handle different Kanban board types consistently

8 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings

@Bonapara
Copy link
Member

Hi @ehconitin, thanks!

I see there is still an issue with the inline-field height (#7163) on this branch. The relation and tag fields are 28px high instead of 24px. All inline fields should be 24px high.

Also, can you shift the floating field slightly to the right? It should be above the name chip rather than sticking to the left border.

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.

Looks great! Can't wait to merge.

@ehconitin
Copy link
Contributor Author

@Bonapara could I get a confirmation on should inline fields be 24px all across the app or just kanban cards?

Thanks :)

@@ -55,11 +55,9 @@ const StyledInlineCellBaseContainer = styled.div`
box-sizing: border-box;
width: 100%;
display: flex;

height: 24px;
Copy link
Member

Choose a reason for hiding this comment

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

Todo for review before merge: check impact as it's probably affecting many places (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! Will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LGTM!

Screenshot from 2024-09-28 14-09-16
Screenshot from 2024-09-28 14-16-48
Screenshot from 2024-09-28 14-17-25

Copy link
Member

Choose a reason for hiding this comment

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

Great!

@bosiraphael bosiraphael self-assigned this Sep 30, 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.

This is great 🙌!

@FelixMalfait FelixMalfait merged commit 0457914 into twentyhq:main Oct 3, 2024
5 of 11 checks passed
Copy link

github-actions bot commented Oct 3, 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 2087:24DA8:A5827C:A6D329:66FEBD9C.
    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'Thu, 03 Oct 2024 15:51:56 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'2087:24DA8:A5827C:A6D329:66FEBD9C'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1727970776'�[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 2087:24DA8:A5827C:A6D329:66FEBD9C.'�[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.4 (linux; x64)'�[39m,
      authorization: �[32m'token [REDACTED]'�[39m
    },
    request: { hook: �[36m[Function: bound bound register]�[39m }
  }
}
danger-results://tmp/danger-results-d8f3029f.json

Generated by 🚫 dangerJS against 2324e62

FelixMalfait added a commit that referenced this pull request Oct 3, 2024
minor follow up fix #7285

---------

Co-authored-by: Félix Malfait <[email protected]>
@ehconitin ehconitin deleted the kanbanCardFollowup branch October 10, 2024 10:47
harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
minor follow up fix twentyhq#7285

---------

Co-authored-by: Félix Malfait <[email protected]>
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.

4 participants