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

Settings cards should use <a> instead of <div> #7356

Closed
Bonapara opened this issue Sep 30, 2024 · 13 comments · Fixed by #7397
Closed

Settings cards should use <a> instead of <div> #7356

Bonapara opened this issue Sep 30, 2024 · 13 comments · Fixed by #7397

Comments

@Bonapara
Copy link
Member

Bonapara commented Sep 30, 2024

Current Behavior

The field type Settings cards use a div container.

CleanShot 2024-09-30 at 17 07 51

This concerns the field type settings cards, but not the emails and calendar settings cards in Accounts (which use an <a> tag), even though they share the same component.

c3713d6712bf11aee76f412aa0c7a7f4924c38eb2e06284424cb4ed830438ba7

Desired Behavior

They should use an <a></a> container for accessibility. Design and behavior should remain the same.

Figma

image

https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=35741-193252&node-type=frame&t=YZppWDu8cDmzxDmJ-11

@sachinks07
Copy link
Contributor

@Bonapara, I would like to work on this issue.

@ehconitin
Copy link
Contributor

@sachinks07 Thanks for the interest!
Could you pick another good first issue

@ehconitin ehconitin self-assigned this Oct 1, 2024
@ehconitin
Copy link
Contributor

@Bonapara

image

These cards are wrapped inside tags because they use an UndecoratedLink component with an href attribute. However, in the field type settings cards, there's no href or navigation link to trigger on click. Instead, they use onClick event handlers for interactivity.

@Bonapara
Copy link
Member Author

Bonapara commented Oct 1, 2024

Can we either use the <button> tag then or add role="button" in the HTML?

@FelixMalfait
Copy link
Member

@ehconitin Actually I just found out we have already have the way to pass the field type through the url so we should be able to change the onClick to a proper link: https://github.com/twentyhq/twenty/blob/main/packages/twenty-front/src/modules/views/view-picker/hooks/useGetAvailableFieldsForKanban.ts#L38

As part of the issue, could you please also:

  • delete all code related to new-field/step-1 (route declaration, story, page), it isn't used anymore
  • new-field/step-2 to just /new-field since there is just one step now

@ehconitin
Copy link
Contributor

looking into it!

@Bonapara
Copy link
Member Author

Bonapara commented Oct 2, 2024

We could also try to fix this one in the same PR: #6967

@ehconitin
Copy link
Contributor

@Bonapara yes, i will try!

@ehconitin
Copy link
Contributor

ehconitin commented Oct 2, 2024

@ehconitin Actually I just found out we have already have the way to pass the field type through the url so we should be able to change the onClick to a proper link: https://github.com/twentyhq/twenty/blob/main/packages/twenty-front/src/modules/views/view-picker/hooks/useGetAvailableFieldsForKanban.ts#L38

As part of the issue, could you please also:

  • delete all code related to new-field/step-1 (route declaration, story, page), it isn't used anymore
  • new-field/step-2 to just /new-field since there is just one step now

@FelixMalfait
That hook wasnt working as it supposed to due to changes in SettingsObjectNewFieldStep2, fixed it!
But there are still two steps, let me explain -
step 1 checks if there are any fields hidden, if not skips to step 2
inside step 2 there were 2 sub steps for filed selection and configuration, which has been recently changed to one step.
So, we should not remove step 1 related code right?

@FelixMalfait
Copy link
Member

@ehconitin yes we should remove entirely the code related to step1, this step was confusing and we have dropped it, it’s not part of the flow anymore. Thanks!

@ehconitin
Copy link
Contributor

We could also try to fix this one in the same PR: #6967

@Bonapara this issue is redundant now right?

@Bonapara
Copy link
Member Author

Bonapara commented Oct 2, 2024

If we fix it in this PR, yes! Can you mention it in your PR so it closes both issues?

@ehconitin
Copy link
Contributor

Yup!

@ehconitin ehconitin moved this from 🆕 New to 👀 In review in 🎯 Roadmap & Sprints Oct 2, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in 🎯 Roadmap & Sprints Oct 9, 2024
harshit078 pushed a commit to harshit078/twenty that referenced this issue Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants