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

Added skeleton loader on page load #3740

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Added skeleton loader on page load #3740

merged 4 commits into from
Feb 8, 2024

Conversation

jeet1desai
Copy link
Contributor

Fix: #3722

Dark
image

Light
image

@la-flor
Copy link

la-flor commented Feb 7, 2024

Passing down a new loading variable is one approach. But I think we can do this without having to add another variable to pass down and keep track of and optionally handle in other places that PageHeader is used.

If we properly handle the concatenation of firstName and lastName in the declaration of the pageName variable here, we can ensure that undefined undefined isn't passed down as a string.

I propose the following declaration of pageName, that we remove the use of the loading variable, and have the skeleton loading be conditional upon title being truthy.

  const pageName =
    objectNameSingular === 'person'
      ? record?.name.firstName || record?.name.firstName
        ? (
            (record?.name.firstName ?? '') +
            ' ' +
            (record?.name.lastName ?? '')
          ).trim()
        : null
      : record?.name;

And if you don't think that code is as legible, you could additionally pull out the name concatenation into its own function.

Reader be aware that I am not a regular contributor (yet). So take my advice as you would like.

@jeet1desai
Copy link
Contributor Author

Thank you @la-flor for your valuable review.

@lucasbordeau, I'd like to hear your thoughts on whether I should use "loading" or "title" as the checker.

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Feb 8, 2024

@jeet1desai That's perfect now, thank you !

@la-flor I think we shouldn't try to guess it here for multiple reasons :

  • It's generally better not to presume anything from a level below, and if in doubt, rely on passing down the semantical intent we have for this component, it's like "Hey PageHeader component put yourself in loading state, I'm managing complex Apollo networking stuff so I'll tell you when loading is finished, while you handle how to nicely display a waiting state for the user"
  • We use PageHeader component to display any text, not just person name field, so we should also let the level above decide what text should be displayed. PageHeader is focused on UI and should be responsible only for displaying the intents of its props.

@lucasbordeau lucasbordeau merged commit ddc5165 into twentyhq:main Feb 8, 2024
10 of 11 checks passed
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.

Avoid showing undefined undefined on show page first load
5 participants