Remote Content Browsing: Clean the library page when displaying other available libraries by AllanOXDi · Pull Request #10216 · learningequality/kolibri

AllanOXDi · 2023-03-09T12:02:33Z

Summary

This PR clean the library page when displaying other available libraries.

References

Reviewer guidance

Please see the figma design

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

github-actions · 2023-03-09T12:21:01Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0a10.dev0_git.61.g4974f665.pex
Windows Installer (EXE)	kolibri-0.16.0a10.dev0+git.61.g4974f665-unsigned.exe
Debian Package	kolibri_0.16.0a10.dev0+git.61.g4974f665-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0a10.dev0+git.61.g4974f665-0.3.0.dmg
Source Tarball	kolibri-0.16.0a10.dev0+git.61.g4974f665.tar.gz
WHL file	kolibri-0.16.0a10.dev0+git.61.g4974f665-py2.py3-none-any.whl

nucleogenesis

Overall code looks good.

I think that making the allDevices prop a bit safer or required would help a bit. Note that making it required will just mean that Vue will complain to you in the devtools whenever you don't pass a value to the prop - so it may just be worth doing null checks before reading into the object in any case.

nucleogenesis · 2023-03-21T06:25:08Z

kolibri/plugins/learn/assets/src/views/LibraryPage/UnPinnedDevices.vue

+      allDevices: {
+        type: Object,
+        required: false,
+        default: null,
+      },


With default: null here you can run into an issue where if the prop is not provided at all it can result in a Error: Cannot read properties of null/undefined in any code that tries to read into allDevices, which they assume is an object.

It doesn't look like you're checking for if it's nullish before you try to read from the object's value (ie, your v-if around line 7) -- so if allDevices really isn't required, then making the default value {} or checking if allDevices is null before reading from it (or maybe using the ?.) would be safer.

nucleogenesis · 2023-03-21T06:28:36Z

kolibri/plugins/learn/assets/src/views/LibraryPage/PinnedNetworkResources.vue

+        if (device['operating_system'] === 'Darwin') {
+          return 'laptop';


Since we default at the end to showing a laptop this seems unnecessary -- is there some particular reason we should check first for "Darwin" before "Android" or could we remove the first if here to the same effect?

akolson · 2023-03-21T11:51:20Z

kolibri/plugins/learn/assets/src/views/LibraryPage/UnPinnedDevices.vue

+    methods: {
+      getDeviceIcon(device) {
+        if (device['operating_system'] === 'Darwin') {
+          return 'laptop';


Please see comment above from @nucleogenesis regarding this.

akolson

Hi @AllanOXDi, Great work. Code generally LGTM. I have left some comments that will help improve the code even further.

akolson · 2023-03-21T12:01:06Z

kolibri/plugins/learn/assets/src/views/LibraryPage/index.vue

Just a general comment on PinnedNetworkResources and PinnedNetworkResources components. It is probably better to load the devices alongside there channels in this file and just pass the results as props to the components since they are in the same file. This comes with benefits like better state management, better reloading capabilities and most importantly implementing the DRY(Don't repeat yourself) principal.

akolson · 2023-03-21T12:02:17Z

kolibri/plugins/learn/assets/src/views/LibraryPage/PinnedNetworkResources.vue

    },
    setup() {
      const { windowBreakpoint } = useKResponsiveWindow();
+      const { baseurl, fetchDevices } = useDevices();


Please see my general comment on the LibraryPage/index.,vue file

akolson · 2023-03-21T12:02:54Z

kolibri/plugins/learn/assets/src/views/LibraryPage/MoreNetworkDevices.vue

Please see my general comment on the LibraryPage/index.,vue file

akolson

Overall code looks good. Thanks @AllanOXDi. Just a final change and we should be good for merging

akolson · 2023-03-23T16:26:53Z

kolibri/plugins/learn/assets/src/views/LibraryPage/index.vue

-          </h2>
+        <div>
+          <h2>{{ $tr('moreLibraries') }}</h2>
          <MoreNetworkDevices />


I think applying the same concept as seen with PinnedNetworkResources would work great. Creating an unpinnedDevices data property and passing it into the PinnedNetworkResources as a prop would help with refreshing state of these devices(I think there is a refresh button somewhere). That would mean also getting rid of the fetchDevices and fetchChannels in the MoreNetworkDevices component as it would not be necessary since you are passing the content(devices and their channels) as props. For now its okay for them to display the same devices. Later we can apply filtering based on results returned from the pinning API that we co-hacked with Jacob yesterday

marcellamaki · 2023-03-28T13:40:38Z

kolibri/plugins/learn/assets/src/views/LibraryPage/UnPinnedDevices.vue

+          return 'laptop';
+        }
+      },
+    },


I had to add another } here when running locally.

marcellamaki · 2023-03-28T13:44:33Z

Hi @AllanOXDi - I was doing a little bit of manual QA on this and I noticed that the channel cards that are displayed here are my channels, not the channels of the remote devices.

Maybe this PR is just for the card implementation, but I just wanted to mention this because I am not sure if it was intentional, and in case we need a follow up issue for it. It looks like the Explore Libraries page that @akolson was working on has this in place, so maybe it's just a matter of a follow up issue using the pattern there.
https://user-images.githubusercontent.com/17235236/228255591-794f7b4a-0471-46d3-b614-14a3c436e7f2.mp4

Nice work overall! Great to see this Library Page coming along so well :)

akolson · 2023-03-28T14:02:00Z

@marcellamaki thanks for raising this. Based on the design spec, i think we shouldn't be having this
@AllanOXDi ?

akolson · 2023-03-30T11:54:43Z

kolibri/plugins/learn/assets/src/views/LibraryPage/index.vue

-          return this.coreString('channelsLabel');
-        }
-      },
+      // ...mapGetters(['isUserLoggedIn']),


This should be removed completely

akolson · 2023-03-30T11:56:18Z

kolibri/plugins/learn/assets/src/views/LibraryPage/index.vue

+      //   this.fetchDevices().then(devices => {
+      //     this.devices = devices.filter(d => d.available);
+      //   });
+      // }


All commented code should be removed completely

akolson · 2023-03-30T11:57:41Z

kolibri/plugins/learn/assets/src/views/LibraryPage/index.vue

+      refresh: {
+        message: 'Refresh',
+        context: 'Link for refreshing library',
+      },


I think adding/moving around translations strings has been suspended for now due to the string freeze. cc @marcellamaki @radinamatic

I think these strings were already in. Might have come due to a rebase yesterday

I just checked and the refresh string already exists in the commonCoreStrings.js. So using this instead and deleting the new one would be an acceptable solution to this.

https://github.com/learningequality/kolibri/blob/develop/kolibri/core/assets/src/mixins/commonCoreStrings.js#L1524

akolson · 2023-03-30T12:05:51Z

kolibri/plugins/learn/assets/src/views/ChannelCard.vue

        required: false,
        default: null,
      },
+      isPinned: {


I think this prop is misleading. I think the wifi icon is meant to specify whether the resource is available or not. @jtamiace , @marcellamaki ?

akolson · 2023-03-30T12:10:03Z

kolibri/plugins/learn/assets/src/views/ChannelCard.vue

+    <div
+      style="text-align:right;margin-right:10px;margin-top:10px"
+    >
+      <KIcon v-if="isPinned" icon="wifi" />


I think the v-if="isPinned" directive should be moved to the parent(div) as hiding just the KIcon will leave additional margins that may not be desirable. Also, see my comments below regarding the isPinned prop

akolson · 2023-03-30T12:14:19Z

kolibri/plugins/learn/assets/src/views/LibraryPage/CardContent.vue

This file should be deleted as its no longer used in light of the discussion here

akolson

Thanks @AllanOXDi! Code looks good. Just a few minor changes and we should be good to go!

AllanOXDi · 2023-03-30T14:39:54Z

Closing this PR again. It's giving me a nightmare updating it.Probably because I deleted a base branch. Please see #10354. Sorry!

github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: small labels Mar 9, 2023

AllanOXDi changed the title ~~fixes device icon~~ Remote Content Browsing: Clean the library page when displaying other available libraries Mar 9, 2023

AllanOXDi requested review from akolson, nucleogenesis and rtibbles March 9, 2023 17:30

github-actions bot added the SIZE: medium label Mar 9, 2023

AllanOXDi added 5 commits March 16, 2023 15:53

fixes device icon

c4f6751

fixes device router

8db2f32

fixed the number of channels not rendering on the cards

73f4ca3

fixes pinned devices channels

fee35c9

removed hard coded pinned devices

9d0a53f

AllanOXDi requested a review from marcellamaki March 16, 2023 14:45

AllanOXDi marked this pull request as ready for review March 16, 2023 14:52

fixed merge conflict

8c80ef3

nucleogenesis requested changes Mar 21, 2023

View reviewed changes

akolson reviewed Mar 21, 2023

View reviewed changes

akolson requested changes Mar 21, 2023

View reviewed changes

AllanOXDi and others added 4 commits March 21, 2023 17:59

made the alldevices prop required

fe83049

removed unncessary check on devices

d30b27f

implemented the dry principal

a2a9d5d

Merge branch 'develop' into remote_content_browsing_functionality

4b9edf3

AllanOXDi requested review from akolson and nucleogenesis March 21, 2023 15:49

fixed failing test

f18797d

akolson requested changes Mar 23, 2023

View reviewed changes

Used props on the morenetworkdevices component

c62027b

AllanOXDi requested a review from akolson March 24, 2023 19:27

akolson approved these changes Mar 27, 2023

View reviewed changes

marcellamaki reviewed Mar 28, 2023

View reviewed changes

fixes chanells not displaying corectly

93c1bd2

AllanOXDi closed this by deleting the head repository Mar 30, 2023

AllanOXDi reopened this Mar 30, 2023

akolson reviewed Mar 30, 2023

View reviewed changes

akolson requested changes Mar 30, 2023

View reviewed changes

AllanOXDi closed this Mar 30, 2023

		if (device['operating_system'] === 'Darwin') {
		return 'laptop';

Conversation

AllanOXDi commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

Uh oh!

github-actions bot commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nucleogenesis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akolson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akolson left a comment

Choose a reason for hiding this comment

Uh oh!

akolson Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcellamaki commented Mar 28, 2023

Uh oh!

akolson commented Mar 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AllanOXDi Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akolson Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akolson left a comment

Choose a reason for hiding this comment

Uh oh!

AllanOXDi commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AllanOXDi commented Mar 9, 2023 •

edited

Loading

github-actions bot commented Mar 9, 2023 •

edited

Loading

akolson Mar 23, 2023 •

edited

Loading

AllanOXDi Mar 30, 2023 •

edited

Loading

akolson Mar 30, 2023 •

edited

Loading

AllanOXDi commented Mar 30, 2023 •

edited

Loading