Skip to content

Remote connection status#10347

Merged
rtibbles merged 8 commits intolearningequality:developfrom
jredrejo:remote_connection_status
Mar 31, 2023
Merged

Remote connection status#10347
rtibbles merged 8 commits intolearningequality:developfrom
jredrejo:remote_connection_status

Conversation

@jredrejo
Copy link
Member

Summary

Adds a connection status icon when browsing a remote library content
Peek 2023-03-29 20-13

References

Closes: #9859

Reviewer guidance

As in the attached video, explore a remote library and connect and disconnect the remote device to test if it works fine.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@jredrejo jredrejo added the TODO: needs review Waiting for review label Mar 29, 2023
@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: medium labels Mar 29, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 2023

@jredrejo jredrejo force-pushed the remote_connection_status branch from d89d276 to 9dad814 Compare March 29, 2023 18:42
@jredrejo jredrejo force-pushed the remote_connection_status branch from fcc2c10 to 27c8929 Compare March 31, 2023 07:32
Copy link
Member

@marcellamaki marcellamaki 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 good to me and initial manual QA checks out. @radinamatic can @pcenov confirm this Monday morning his time?

</div>
</template>
</CoreMenu>
<DeviceConnectionStatus :deviceId="deviceId" />
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking, but just double checking you intentionally left out the color prop here :)

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Thanks @jredrejo. This looks good and the implementation is very clear.

@rtibbles rtibbles merged commit 1e6ddcf into learningequality:develop Mar 31, 2023
@pcenov
Copy link
Member

pcenov commented Apr 3, 2023

Hi @jredrejo, I have some questions because while I am able to see the 'Disconnected' status icon as shown in your example I am also observing the following issues and it's not clear to me whether these are in scope of this PR or are separate issues:

  1. Why is the 'Disconnected' status icon being displayed for channels and resources which are on my own device?
  2. When I click on a channel on the remote device I can't see the channel's resources and therefore I can't test whether the 'Disconnected' status icon is being displayed there while viewing the resources as specified in Remote Content Browsing: Update UI to display connection errors #9859. The following error is displayed in the console: TypeError: Cannot read properties of undefined (reading 'page')
  3. When I refresh the 'Explore libraries' page for a moment I see the 'Disconnected' status icon.

Video:

2023-04-03_13-16-00.mp4

@jredrejo jredrejo mentioned this pull request Apr 6, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: medium TODO: needs review Waiting for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remote Content Browsing: Update UI to display connection errors

5 participants