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

Migrate to twenty-ui - navigation/link #7837

Merged
merged 7 commits into from
Oct 22, 2024
Merged

Migrate to twenty-ui - navigation/link #7837

merged 7 commits into from
Oct 22, 2024

Conversation

gitstart-app[bot]
Copy link
Contributor

@gitstart-app gitstart-app bot commented Oct 18, 2024

This PR was created by GitStart to address the requirements from this ticket: TWNTY-7535.


Description.

Migrate link components to twenty-ui

Fixes #7535

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 migrates various link components to the 'twenty-ui' library across multiple files in the twentyhq/twenty repository. Here's a concise summary of the key changes:

  • Updated import statements to use 'twenty-ui' for components like UndecoratedLink, ActionLink, RoundedLink, and Toggle
  • Removed local imports for these components and replaced them with 'twenty-ui' imports
  • Maintained existing functionality while standardizing UI components
  • Minor adjustments to component usage and prop names in some cases
  • Removed unnecessary React imports in some files

Key points to consider:

  • Ensure consistent usage of 'twenty-ui' components across the codebase
  • Review any TODO comments left in the code, particularly in PhoneDisplay.tsx
  • Verify that all migrated components maintain their original functionality
  • Consider updating documentation to reflect the new 'twenty-ui' usage

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

@charlesBochet
Copy link
Member

@gitstart-twenty could you rebase this one?

@charlesBochet charlesBochet self-assigned this Oct 21, 2024
@charlesBochet
Copy link
Member

@gitstart-twenty I have rebased your PR, could you fix the stories

@charlesBochet
Copy link
Member

@Devessier could you take a look at this branch (we actually have the problem on main but let's solve it here to avoid fixing conflicts)

  • we have refactored the Toggle to wrap it in a internally
  • on CalendarSettings page we want the label to actually be the whole row to trigger the switch
image

The issue is that the click event is bubbled twice

I guess the recommended way would be to remove the additional onClick on the row and to rely on labels. Curious to see how you would solve this

@Devessier
Copy link
Contributor

@Devessier could you take a look at this branch (we actually have the problem on main but let's solve it here to avoid fixing conflicts)

  • we have refactored the Toggle to wrap it in a internally
  • on CalendarSettings page we want the label to actually be the whole row to trigger the switch
image The issue is that the click event is bubbled twice

I guess the recommended way would be to remove the additional onClick on the row and to rely on labels. Curious to see how you would solve this

Below, I list the things I asked myself or thought while reading the code:

  • When developers find themselves writing <div onClick={} />, they should think about it twice. I'm not an accessibility expert, and sometimes it's easier to have two versions coexist, one accessible and the other not. That's to say that clickable divs should be prohibited most of the time, but they are exceptions, and I'm open to discussing them.
  • We don't wait for the Toggle's onChange event here. The input element should be the single source of truth.
    <StyledToggle
    value={calendarChannel.isContactAutoCreationEnabled}
    />
    </SettingsOptionCardContent>
  • A simple approach is only to toggle the input when the text describing the input best is clicked. Example: GitHub
    CleanShot 2024-10-22 at 15 19 46@2x
  • We could render a <label> element for the <Section />. That way, the whole element would toggle the input easily. The issue is that <label> elements can only contain phrasing content, that is, no <div>.
  • Another solution would be to create a <label> element that we style to take the all height and width. That's a trick I often use for links: https://stackoverflow.com/a/74063425. In Tailwind-like code:
<div class="relative">
  <label for="autocreation">
    Auto-creation

    <span class="absolute inset-0"><span>
  </label>

  <!-- ... -->

  <Toggle id="autocreation" />
</div>

I would go with the latest solution, as shown here: 346433a. It works as expected:

CleanShot.2024-10-22.at.16.17.58.mp4

What is essential is that the label has a meaningful name. Using "Auto-creation" would be good. There are more evolved solutions, but I would only be willing to use them if I rely on an a11y library. These solutions are helpful when you want to build truly custom inputs.

CleanShot 2024-10-22 at 16 16 23@2x

@charlesBochet charlesBochet merged commit 4306444 into main Oct 22, 2024
17 of 18 checks passed
@charlesBochet charlesBochet deleted the TWNTY-7535 branch October 22, 2024 15:36
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