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

Spinner #214

Merged
merged 6 commits into from
Dec 3, 2023
Merged

Spinner #214

merged 6 commits into from
Dec 3, 2023

Conversation

phamkelly17
Copy link
Contributor

Notion task link

n/a

Implementation description

  • Before, the header of the table would appear on the screen before the table data was available. Table data would appear at a delay.
  • adding a spinner if we're in the process of fetching table data
  • adding a "no results found" message if no results are founf

Steps to test

  1. go to home page
  2. check if the spinner is there and looks good while the table is loading
  3. make sure the table loads
  4. filter so that there are 0 results and check if it gives a message that says "No results found."
  5. repeat for resident and employee directory pages (step 4 not necessary)

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR
  • If I have made API changes, I have updated the REST API Docs
  • IF I have made changes to the db/models, I have updated the Data Models Page
  • I have documented this PR in the Production Release Page
  • I have updated other Docs as needed

speed="0.65s"
emptyColor="gray.200"
size="xl"
/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no way to put these props in the default props section in the SpinnerStyles.tsx file. it just won't work. trust me, me and gpt have tried

Copy link
Collaborator

Choose a reason for hiding this comment

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

skill issue

Copy link

github-actions bot commented Dec 2, 2023

Visit the preview URL for this PR (updated for commit c8b135f):

https://blueprintsupportivehousing--pr214-kelly-spinner-gtn7bdnt.web.app

(expires Sun, 10 Dec 2023 19:46:17 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: f6bcdba7452bf82a6ec1a299c37d1bdff7870d09

@phamkelly17 phamkelly17 changed the title Kelly/spinner Spinner Dec 2, 2023
Copy link
Collaborator

@connor-bechthold connor-bechthold left a comment

Choose a reason for hiding this comment

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

this looks great however it would be nice if this spinner could also show when something is being added/edited/deleted (since it triggers a refetch of the table)

what you can do is have a loading state that's initially set to false. everytime you refetch new records (i.e. call getLogRecords()), you can set the loading state to True before the call, and then False after the call

ex.

setIsLoading(True)
const data = await LogRecordAPIClient.filterLogRecords({
      buildingId: buildingIds,
      employeeId: employeeIds,
      attnTo: attentionToIds,
      dateRange: dateRange[0] === "" && dateRange[1] === "" ? [] : dateRange,
      residents: residentsIds,
      tags: tagsValues,
      flagged,
      resultsPerPage,
      pageNumber,
    });
setIsLoading(False)

that way the spinner can show on inital page renders AND adds/edits/deletes. the rest of the logic you implemented should be able to stay the same

@@ -375,7 +375,7 @@ const EditLog = ({
placeholder="Select Residents"
onChange={handleResidentsChange}
defaultValue={residentOptions.filter(
(item) => logRecord.residents.includes(item.label),
(item) => logRecord.residents && logRecord.residents.includes(item.label),
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there some sort of error that appears that required this to be changed? residents should never be null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't recall changing this file in this PR, but yeah there is some error that requires this to be changed. i believe owen made a pr for it

speed="0.65s"
emptyColor="gray.200"
size="xl"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

skill issue

Comment on lines 74 to 80
{numUsers < 0 ? (
<Spinner
thickness="4px"
speed="0.65s"
emptyColor="gray.200"
size="xl"
/>
Copy link
Contributor

@kevin-pierce kevin-pierce Dec 3, 2023

Choose a reason for hiding this comment

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

So as Connor mentioned, the best way to implement this is usually using a state variable that simply keeps track of the loading state as a boolean ; we then would conditionally render the spinner only if "loading" is taking place.

Here's a working snippet

// Initially set to false ; this way by DEFAULT we display the spinner
const [loadedUsers, setLoadedUsers] = useState(false)

// At the "onMount" useEffect (the one w no dependencies), we'd want to set the loaded users state to true
// The only reason I don't want to set it in the other useEffect as well is because from what I understand, we don't want to display the spinner for every batched fetch of users
  useEffect(() => {
    countUsers();
    setLoadedUsers(true)
  }, []);

// Then below in the JSX:
{!loadedUsers ? <Spinnner> : <other stuff>}

Copy link
Contributor

@kevin-pierce kevin-pierce left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Collaborator

@connor-bechthold connor-bechthold left a comment

Choose a reason for hiding this comment

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

lgtm!!!

@phamkelly17 phamkelly17 merged commit 91140b7 into main Dec 3, 2023
3 checks passed
@phamkelly17 phamkelly17 deleted the kelly/spinner branch December 3, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants