Allow renaming names from uploaded files #6358

ehconitin · 2024-07-21T20:56:45Z

Done with issue #6317. I had to create a new component, EditableField (Component), because the existing InputText component has a fixed height (link), which causes the attachment row to increase in size everytime clicked on rename.

I’ve also updated the colors for the box border and box shadow as specified in Figma.

Please let me know your thoughts.
Thanks

2024-07-22.02-25-40.mp4

greptile-apps

PR Summary

The pull request introduces a new feature allowing users to rename uploaded files directly within the application, along with some UI enhancements.

New Component: Added EditableField.tsx in packages/twenty-front/src/modules/ui/field/input/components/ for inline renaming functionality.
Attachment Dropdown: Updated AttachmentDropdown.tsx to include a 'Rename' option with a corresponding handleRename function.
Attachment Row: Modified AttachmentRow.tsx to integrate the EditableField component for renaming files inline.
Theme Updates: Added editable color and box shadow styles in packages/twenty-ui/src/theme/constants/BorderDark.ts, BorderLight.ts, BoxShadowDark.ts, and BoxShadowLight.ts.
UI Consistency: Ensured new styles align with Figma specifications for a cohesive user experience.

_{7 file(s) reviewed, 1 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-07-21T20:57:41Z

packages/twenty-front/src/modules/ui/field/input/components/EditableField.tsx

+          onFocus={onFocus}
+          onBlur={onBlur}
+          onChange={(event: ChangeEvent<HTMLInputElement>) => {
+            onChange?.(event.target.value);


Style: Add error handling for the onChange callback to prevent potential runtime errors.

Bonapara · 2024-07-22T08:38:41Z

Thanks, @ehconitin! It would be great if the text was pre-selected. This should be an option that could be applied to any TextInput focus field.

ehconitin · 2024-07-22T10:58:47Z

On it. @Bonapara

ehconitin · 2024-07-22T11:23:30Z

@Bonapara . Done.
thanks for the feedback.

2024-07-22.16-50-42.mp4

lucasbordeau · 2024-07-22T13:18:12Z

packages/twenty-front/src/modules/activities/files/components/AttachmentRow.tsx

+  const handleInputBlur = () => {
+    handleUpdate();
+  };
+  useEffect(() => {


This logic should be inside the EditableField component

lucasbordeau · 2024-07-22T13:18:36Z

packages/twenty-front/src/modules/activities/files/components/AttachmentRow.tsx

+    setIsEditing(true);
+  };
+
+  const handleInputChange = (text: string) => {


Same here, should be inside the editable field component

lucasbordeau · 2024-07-22T13:21:21Z

packages/twenty-front/src/modules/ui/field/input/components/EditableField.tsx

+  onBlur?: FocusEventHandler<HTMLInputElement>;
+};
+
+const EditableFieldComponent = (


Seems like some logic could be shared with TextInput but let's see this later.

ehconitin · 2024-07-22T16:11:46Z

@lucasbordeau Made the suggested changes.
Refactored the EditableField component to handle its own state and passed the updated function signature for handleUpdate. This ensures better encapsulation.
Thanks for the review.

Weiko

LGTM!

github-actions · 2024-08-05T15:22:22Z

	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 8087:1613A3:A78E96B:136DF459:66B0EE2D.
    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'Mon, 05 Aug 2024 15:22:21 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'8087:1613A3:A78E96B:136DF459:66B0EE2D'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1722871401'�[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 8087:1613A3:A78E96B:136DF459:66B0EE2D.'�[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-323ea5ac.json

Generated by 🚫 dangerJS against 33c3448

ehconitin · 2024-08-05T16:19:06Z

Hi @FelixMalfait,

Thanks for merging my PR and optimizing the code! I noticed a couple of features from the original issue didn’t make it into the final merge:

Filenames aren’t preselected as planned.
The input box border doesn’t match the Figma design.

These were part of my initial commit but seem to have been lost during the optimization. Should I open a new issue to reintroduce these, or do you have suggestions on how we can incorporate them without duplicating code?

Looking forward to your thoughts!

FelixMalfait · 2024-08-06T06:23:18Z

@ehconitin I'm sorry I forgot to give feedback.
I think the direction you took made sense, and it would have been hard for you to do weight the trade-off yourself.
What I didn't like is that you introduced duplicated code, and even with that duplicated code some features didn't work out of the box (e.g. press escape to exit field).

The EditableField file wasn't properly named imo. It could have been something like FileInputField. But then we introduce a new pattern because FileInput is not a field type unlike other xxxField. So that would make the codebase more confusing.

Sometimes we have, as engineers, a duty to call out to the design team that they're asking for too much and that their ask will create a debt in the code. Then we can decide together if it's worth creating that debt.

In that case:

We don't have yet a clear vision for what the widgets on the show page are going to look like.
We don't have yet a clear vision of what the attachments will look like 6 months from now.

You might have seen recently we removed a lot of code and migrated Tasks/Notes to become standard objects in the code base. That's in line with our general vision in this project which is to rely on as few primitives as possible to power all concepts throughout the app. Attachments are likely to undergo a similar migration where we try to make them fit into standard patterns as much as possible.

Thanks a lot for your contribution! It was still helpful to have explored this direction

cc @Bonapara you'll notice the design are not like you wanted sorry!

ehconitin · 2024-08-06T09:22:47Z

Hey @FelixMalfait ,

Thanks for the feedback! I appreciate you pointing out the duplicated code and the naming issues. I see where you're coming from about the design debt, too.

I’ll look into these and work on improving things.

Thanks alot!

ehconitin added 2 commits July 22, 2024 01:39

Allow renaming names from uploaded files

56bf16b

Allow renaming names from uploaded files

44e4f93

greptile-apps bot reviewed Jul 21, 2024

View reviewed changes

ehconitin mentioned this pull request Jul 21, 2024

Allow renaming names from uploaded files #6317

Closed

ijreilly added the -PR: awaiting author label Jul 22, 2024

ehconitin added 2 commits July 22, 2024 16:41

Allow renaming names from uploaded files - auto select

c8a2c87

Allow renaming names from uploaded files - auto select

4596fae

lucasbordeau reviewed Jul 22, 2024

View reviewed changes

lucasbordeau self-assigned this Jul 22, 2024

Allow renaming names from uploaded files

1d1c846

FelixMalfait added 3 commits August 5, 2024 15:59

Merge main

a15ccaf

Simplify to avoid duplicated code

d3cfebe

Remove unused

33c3448

Weiko approved these changes Aug 5, 2024

View reviewed changes

Weiko merged commit ed0102e into twentyhq:main Aug 5, 2024
5 of 11 checks passed

ehconitin deleted the fixed-issue-6317 branch August 6, 2024 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow renaming names from uploaded files #6358

Allow renaming names from uploaded files #6358

ehconitin commented Jul 21, 2024

greptile-apps bot left a comment

greptile-apps bot Jul 21, 2024

Bonapara commented Jul 22, 2024 •

edited

Loading

ehconitin commented Jul 22, 2024

ehconitin commented Jul 22, 2024 •

edited

Loading

lucasbordeau Jul 22, 2024

ehconitin Jul 22, 2024

lucasbordeau Jul 22, 2024

ehconitin Jul 22, 2024

lucasbordeau Jul 22, 2024

ehconitin commented Jul 22, 2024

Weiko left a comment

github-actions bot commented Aug 5, 2024

ehconitin commented Aug 5, 2024 •

edited

Loading

FelixMalfait commented Aug 6, 2024

ehconitin commented Aug 6, 2024

Allow renaming names from uploaded files #6358

Allow renaming names from uploaded files #6358

Conversation

ehconitin commented Jul 21, 2024

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jul 21, 2024

Choose a reason for hiding this comment

Bonapara commented Jul 22, 2024 • edited Loading

ehconitin commented Jul 22, 2024

ehconitin commented Jul 22, 2024 • edited Loading

lucasbordeau Jul 22, 2024

Choose a reason for hiding this comment

ehconitin Jul 22, 2024

Choose a reason for hiding this comment

lucasbordeau Jul 22, 2024

Choose a reason for hiding this comment

ehconitin Jul 22, 2024

Choose a reason for hiding this comment

lucasbordeau Jul 22, 2024

Choose a reason for hiding this comment

ehconitin commented Jul 22, 2024

Weiko left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 5, 2024

Log

ehconitin commented Aug 5, 2024 • edited Loading

FelixMalfait commented Aug 6, 2024

ehconitin commented Aug 6, 2024

Bonapara commented Jul 22, 2024 •

edited

Loading

ehconitin commented Jul 22, 2024 •

edited

Loading

ehconitin commented Aug 5, 2024 •

edited

Loading