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

feat: Add loading indicator to Manage Domains sidebar #9142

Merged
merged 4 commits into from
Nov 16, 2023

Conversation

sumitappt
Copy link
Contributor

Checklist

@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Oct 30, 2023
</Form.Item>
</Form>
<Form component={false}>
<Form.Item>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM.

Do you mind to add screenshots to this PR or a video recording so we can see what the fix looks like?

@sumitappt
Copy link
Contributor Author

Overall LGTM.

Do you mind to add screenshots to this PR or a video recording so we can see what the fix looks like?
Here it is
Screencast from 31-10-23 04_51_07 PM IST.webm

@maggiehays maggiehays added the hacktoberfest-accepted Acceptance for hacktoberfest https://hacktoberfest.com/participation/ label Oct 31, 2023
onFocus={() => setIsFocusedOnInput(true)}
dropdownStyle={isShowingDomainNavigator ? { display: 'none' } : {}}
loading={loading}
disabled={loading}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we definitely disable this if it's not done loading yet? for example, if they want to search for a specific domain, they should probably be able to do that right away?

not blocking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As soon as the user opens the modal, we kick off an API call to fetch the Domains. Since the user won't be typing anything initially, it can be a bit confusing to see an empty list when they first focus on the search field.

To avoid this confusion, we disable the search field and show a loader to signal that we're in the process of loading the initial list. Once the domains are ready, we enable the search functionality, allowing the user to start their search.

Even while they're searching, if they momentarily pause typing, we have a timer in place to trigger a search API call. During this time, the search field remains disabled with a loader, indicating that we're actively fetching results that match their input.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be resolved now. No longer any disabled state, as discussed during our standup

@sumitappt
Copy link
Contributor Author

@jjoyce0510

Here the screen shots of PR changes

DomainNavigator Empty State

DomainNavigator Empty State

Select options loading indicator
Screenshot from 2023-11-08 10-10-15

Select options Empty State
Select Option Empty State

@jjoyce0510
Copy link
Collaborator

Looks good - just want to confirm that when user first opens the modal, they will not see a disabled state, right? They should just see loading indicator as we load the browser.

>
{domainSearchOptions}
{loading ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor refactoring request, but otherwise this looks ready to go!

@sumitappt
Copy link
Contributor Author

Looks good - just want to confirm that when user first opens the modal, they will not see a disabled state, right? They should just see loading indicator as we load the browser.

Yes we removed the disable state

@jjoyce0510 jjoyce0510 merged commit e6305c0 into datahub-project:master Nov 16, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Acceptance for hacktoberfest https://hacktoberfest.com/participation/ product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants