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

Custom fields lacks empty tag #7250

Closed
harshit078 opened this issue Sep 25, 2024 · 13 comments
Closed

Custom fields lacks empty tag #7250

harshit078 opened this issue Sep 25, 2024 · 13 comments
Labels
good first issue Good for newcomers scope: front Issues that are affecting the frontend side only

Comments

@harshit078
Copy link
Contributor

Bug Description

  • Custom fields lacks empty tag as a result they have very limited clickable area to edit them.
  • This creates a lot of whitespaces in front of fields which hinders the design

Current behavior

Screenshot 2024-09-25 at 4 35 00 PM Screenshot 2024-09-25 at 4 54 13 PM

Expected

There should be empty tags in front of custom fields when empty

@harshit078 harshit078 changed the title Ex: In DarkMode, a blank square appears in bottom right corner while scrolling Custom fields lacks empty tag Sep 25, 2024
@Bonapara
Copy link
Member

I cannot reproduce the bug 🤔 Any idea/ more context?

CleanShot 2024-09-25 at 14 00 31

@harshit078
Copy link
Contributor Author

There was a mistake from my side on providing enough context @Bonapara. As you can see in the screen recording below, custom fields create empty spaces under company.

Screen.Recording.2024-09-25.at.6.31.32.PM.mov

@Bonapara
Copy link
Member

Could reproduce for relation custom fields in dropdowns, thanks @harshit078!

@FelixMalfait FelixMalfait added the good first issue Good for newcomers label Oct 14, 2024
@FelixMalfait
Copy link
Member

/oss.gg 200

Copy link

oss-gg bot commented Oct 14, 2024

Thanks for opening an issue! It's live on oss.gg!

@FelixMalfait FelixMalfait added scope: front Issues that are affecting the frontend side only and removed 🕹️ oss.gg labels Oct 14, 2024
@nganphan123
Copy link
Contributor

Hi, is this available to be taken? Can I work on this?

@nganphan123
Copy link
Contributor

Hi @Bonapara , I did some inspection and it seems like the Empty tag is missing for Multi-select and One-to-many data type. The problem lies in the check isDisplayModeContentEmpty for empty content, which is referring to function isFieldValueEmpty()

{(isDisplayModeContentEmpty && !shouldDisplayEditModeOnFocus) ||
!children ? (
<StyledEmptyField>{emptyPlaceHolder}</StyledEmptyField>
) : (
children
)}

From what I understand, the isFieldValueEmpty() doesn't properly check if the array is empty. My solution would be adding changes to isFieldValueEmpty() to check the empty array for fields multi-select and relation.

Please let me know if I'm on the right track or if there's anything I've misunderstood. Thank you!

@Bonapara
Copy link
Member

@lucasbordeau WDYT?

@FelixMalfait
Copy link
Member

@nganphan123 yes it seems you're on the right track! Thank you!

@FelixMalfait
Copy link
Member

@nganphan123 fyi we never really shiped relationToOne in the product in the end.

I'd probably do something like this instead (not tested):

if (isFieldRelation(fieldDefinition)) {
   if(isFieldRelationToOneValue(fieldValue)) {
       return isValueEmpty(fieldValue);
   }
    return  !isNonEmptyArray(fieldValue);
}

Close to what you're saying but better handling of RelationToOne imo (even though we don't use it)

@nganphan123
Copy link
Contributor

@FelixMalfait I see. I added the change and it passed the tests. Also, isFieldRelationToOneValue and isFieldRelationFromManyValue have the same implementation so it didn't identify which field is RelationToMany, so I changed to isArray instead.

  if (isFieldRelation(fieldDefinition)) {
    if (isArray(fieldValue)) {
      return !isNonEmptyArray(fieldValue);
    }
    return isValueEmpty(fieldValue);
  }

FelixMalfait added a commit that referenced this issue Oct 21, 2024
This PR fixes this issue #7250

---------

Co-authored-by: Félix Malfait <[email protected]>
@BOHEUS
Copy link
Contributor

BOHEUS commented Nov 2, 2024

@FelixMalfait this issue can be closed since changes are merged

@FelixMalfait
Copy link
Member

Thanks @BOHEUS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers scope: front Issues that are affecting the frontend side only
Projects
None yet
Development

No branches or pull requests

5 participants