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

Remove step 1 of new object field #7397

Merged
merged 21 commits into from
Oct 9, 2024

Conversation

ehconitin
Copy link
Contributor

@ehconitin ehconitin commented Oct 2, 2024

fixes #7356
fixes #6967
fixes #7102
fixes #7121
fixes #7505

@ehconitin ehconitin marked this pull request as ready for review October 2, 2024 18:42
@ehconitin
Copy link
Contributor Author

@Bonapara removed all the redundant icons code beside breadcrumb! Those Icons are removed right?

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 addresses three main issues in the Twenty application's settings pages:

  • Improves accessibility by replacing <div> with <a> tags for settings cards
  • Simplifies the new field creation process by removing the two-step approach
  • Reduces settings page flashing during loading with a skeleton loader

Key changes:

  • Introduced SettingsSkeletonLoader component for improved loading experience
  • Refactored SettingsObjectNewField to combine field type selection and configuration into a single step
  • Removed icons from SubMenuTopBarContainer components across multiple settings pages
  • Updated routing to reflect the simplified new field creation process
  • Adjusted SettingsDataModelFieldTypeSelect for better accessibility and user experience

30 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings

@Bonapara
Copy link
Member

Bonapara commented Oct 3, 2024

@Bonapara removed all the redundant icons code beside breadcrumb! Those Icons are removed right?

Thanks Nitin! Do you have a screenshot so I can better understand what you mean?

@ehconitin
Copy link
Contributor Author

ehconitin commented Oct 3, 2024

@Bonapara removed all the redundant icons code beside breadcrumb! Those Icons are removed right?

Thanks Nitin! Do you have a screenshot so I can better understand what you mean?

Untitled design (3)

@Bonapara
There used to be icons here right? and they are now removed I think, because I cant see them even in Figma.
But the code related to it was not cleaned. Just making sure I should clean it or not!

@Bonapara
Copy link
Member

Bonapara commented Oct 3, 2024

Yes you can clean it! Thanks @ehconitin

@FelixMalfait
Copy link
Member

Thank you! This works but I feel we could do even better :)

When you open links in a new tab, it's not working because you created fake links to = #
Instead we should probably link to={?fieldType=${key}}
We should also be able to use the backed button (remove replace: true)

And that will be an issue because if you press the back button, configureStep remains with its previous value.
So I would advocate for a bigger refactoring, either keep the current code and drop isConfigureStep to just base it on whether the field type parameter has been provided in the url, or just split into two pages.

Do you see what I mean @ehconitin?

@ehconitin
Copy link
Contributor Author

ehconitin commented Oct 3, 2024

@FelixMalfait Yes!
Refactoring it to two separate pages would be much cleaner!
I am dropping isConfigureStep state, its very restrictive.
Couldnt do to={?fieldtype=${key}} because since its the same page, had to add a useEffect to keep listening for url changes, so opted for a onClick navigation, which is kind of a cheap trick.
Will refactor!
Thanks :)

@ehconitin
Copy link
Contributor Author

@FelixMalfait conflicts made this way more tricky! :(
Refactor is done! Please let me know if I have missed anything.

@ehconitin
Copy link
Contributor Author

sb-test failed, checking!

@ehconitin
Copy link
Contributor Author

Ready for review!

@Bonapara
Copy link
Member

Bonapara commented Oct 9, 2024

I just checked from a product perspective, and it looks good! Thanks for the great work, @ehconitin.

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.

Great work! I think we should merge this as the PR is already big!
One potential improvements would have been to disable the configure step if no fieldType is passed instead of defaulting to text which can sometimes create weird behavior (e.g. you're on step 2 you come back to step 1 via top bar to check what was selected then come back to step 2 via top bar - your field type has changed). Not a big deal / TBC once we have experience with users!

@FelixMalfait FelixMalfait merged commit 58cbcbf into twentyhq:main Oct 9, 2024
11 checks passed
Copy link

github-actions bot commented Oct 9, 2024

Thanks @ehconitin for your contribution!
This marks your 43rd PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

@ehconitin
Copy link
Contributor Author

ehconitin commented Oct 9, 2024

@FelixMalfait
Ah, I missed that one!
It was already done in #6969 but we scrapped it because keyboard navigation was not required anymore.
I forgot to address it here.
I will address this in some minor fix PR soon!

Thanks!

FelixMalfait added a commit that referenced this pull request Oct 9, 2024
context -
#7397 (review)
P.S. Apologies for the background music in the screen recording—I didn’t
realize my mic was on while capturing it. 😅


https://github.com/user-attachments/assets/0cd31aa7-9ce2-4577-a79a-73c9890f2905

---------

Co-authored-by: Félix Malfait <[email protected]>
@ehconitin ehconitin deleted the removeStep1NewField branch October 10, 2024 10:43
harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
context -
twentyhq#7397 (review)
P.S. Apologies for the background music in the screen recording—I didn’t
realize my mic was on while capturing it. 😅


https://github.com/user-attachments/assets/0cd31aa7-9ce2-4577-a79a-73c9890f2905

---------

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
Projects
None yet
4 participants