Onboarding import facility - Mostly designs #9534

nucleogenesis · 2022-06-23T23:33:54Z

Summary

Applies new OnboardingStepBase styles to Import Facility flow

onboarding-import-flow-designs.mp4

Moves syncTaskUtils into core as it's used across multiple plugins.
Avoids errors re: using i18n before it is ready by assigning bound functions to the t8n maps. This could be done instead by making those thingToTranslationMap objects into functions that return objects but I did this first and feels like half-dozen in one hand and six in the other in this case.
Applying style and data management pattern with ImportFacilitySetup as a coordinator component
Adds event listener and handler to OnboardingStepBase so that you can hit enter to continue while having an INPUT element focused (any form element, radio button).

Not included in this PR because the main focus is applying designs

Import actually working
Robust a11y review re: keyboard navigation, focus trapping modals, etc - needs testing
Proper handling of when and how users can "Go back" at any point and how to display buttons/options in the case of a failed import and during the importing loading process (ie, what buttons to show) some tests are skipped right now until we work this out

References

Fixes #9310

Reviewer guidance

Please walk through the flow OR watch the video above. If you get stuck, wipe your browser's local data and refresh the page.

Do designs look to spec?
a11y notes would be much appreciated, although fixes for them will be put into a separate issue and fixed in follow up

Code review: FWIW I tried to make my commits pretty self-contained overall

Is it clear why the functions are bound when defined in the now moved to core syncTaskUtils module? Should those maps just be returned by a function instead?
Does the move, generally, make architectural sense?

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 · 2022-06-23T23:48:21Z

Build Artifacts

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

radinamatic · 2022-06-28T02:37:27Z

Designs look to spec, and I found 2 missed instances of the proper focus trap:

Language selector - if the user presses the Tab key after the CONFIRM button the focus should return to the top, at the first language radio button (which allows to use arrow keys to potentially select another language), but instead it exits the modal and ends up in the browser address bar.
When the user selects the option to import a facility and presses the Enter key while the CONTINUE button is in focus, Select network address modal should trap the focus, but instead it is again located behind, in the browser address bar.

Question for @jtamiace (added a comment in Figma too): Wouldn't it make more sense that in case where there are devices detected at this step, the Add new address link came after the detected devices in the network?

radinamatic · 2022-06-28T02:41:59Z

Radio button text should overflow to the next line, not be cut out with ellipsis.

@marcellamaki Is that something that we should fix in the KDS component?

marcellamaki · 2022-06-28T14:37:28Z

Radio button text should overflow to the next line, not be cut out with ellipsis.

@radinamatic - yes, this is something we should fix in KDS. I am going to work on putting together a KDS dev issues milestone in support of 0.16 for us to review before our retro. I think we have an open issue for this, but will double check!

marcellamaki

Is it clear why the functions are bound when defined in the now moved to core syncTaskUtils module?

It is not clear to me, but overall reading through the code, I did find it easy enough to follow?

As for whether moving is a good idea, I think on a basic level, having the things in a core util is helpful after a certain point, but I don't know enough context of if there are cons to this. would defer to @rtibbles on that one.

My other comments are small and not blocking. Design/figma look good to me

marcellamaki · 2022-06-28T13:41:38Z

kolibri/core/assets/src/utils/syncTaskUtils.js

+
+export const TaskTypes = {
+  REMOTECHANNELIMPORT: 'kolibri.core.content.tasks.remotechannelimport',
+  REMOTECONTENTIMPORT: 'kolibri.core.content.tasks.remotecontentimport',


Not blocking: This is a bit nit-picky, and I can see that this was just a "move existing code" but if it's possible, would we have these be more like REMOTE_CHANNEL_IMPORT without it being too much of a hassle? I think it would be a lot easier to read. If easier to keep as is, because these are in too many places, no problem though

💯 no prob at all good idea

marcellamaki · 2022-06-28T13:41:54Z

kolibri/core/assets/src/utils/syncTaskUtils.js

+
+export const TransferTypes = {
+  LOCALEXPORT: 'localexport',
+  LOCALIMPORT: 'localimport',


Same as above

marcellamaki · 2022-06-28T13:50:34Z

kolibri/plugins/setup_wizard/assets/src/views/OnboardingStepBase.vue

@@ -139,6 +147,15 @@
        return availableLanguages[currentLanguage];
      },
    },
+    methods: {
+      /* If the user is focused on a form element and hits enter, continue */
+      handleEnterKey(e) {


nice! not for now, but I do wonder if we should implement this more widely?
it is my personal number 1 pet peeve when typing in a text field that enter is not the same as continue (if I'm not focused on the button)

jtamiace · 2022-06-28T19:49:38Z

Wouldn't it make more sense that in case where there are devices detected at this step, the Add new address link came after the detected devices in the network?

Yes I agree, and it looks like this is the layout for the other "Select network address" modals already existing in Kolibri. Made the change to all the modals for onboarding in Figma in case that's still helpful

rtibbles

.bind(null... feels a little opaque.

Wonder if we could start to experiment with another way of sharing dependencies that doesn't overload the core API?

rtibbles · 2022-06-28T21:18:15Z

kolibri/core/assets/src/utils/syncTaskUtils.js

-import bytesForHumans from 'kolibri.utils.bytesForHumans';
-import { TaskStatuses, TaskTypes } from '../constants';
+import commonCoreStrings from '../mixins/commonCoreStrings';
+import taskStrings from '../mixins/taskStrings';


I know it was like this when you got here, but for the taskStrings at least, the mixins module exposes an export for the function:

kolibri/kolibri/core/assets/src/mixins/taskStrings.js

Line 119 in 60dd14c

export function getTaskString(...args) {

Which should avoid having to do an awkward destructing off the methods property of the mixin itself.

Would be good to do something similar for commonCoreStrings.

The coreString signature maps directly to $tr a la (key : string, args: object) rather than taking some arbitrary number of nameless args - I'll apply the same here to make it more readable / consistent with the other code.

The commonCoreStrings pattern though for using it in JS is like:

import { coreStrings } from 'kolibri...commonScoreStrings'; // imports the created translator coreStrings.$tr(...);

So maybe it'd be good to export a getter function there as well and unify the pattern altogether. In which case maybe I can move this to a separate issue re: strings things to do before release?

Yeah, I think having the explicit args is better - might be good to open a follow up and hash it out there, yes. I can imagine there will probably be other things to consider before we start moving stuff around more!

rtibbles · 2022-06-28T21:22:10Z

kolibri/core/assets/src/utils/syncTaskUtils.js

-  [TaskStatuses.CANCELED]: getTaskString('taskCanceledStatus'),
-  [TaskStatuses.CANCELING]: getTaskString('taskCancelingStatus'),
-  [TaskStatuses.FAILED]: getTaskString('taskFailedStatus'),
+  [TaskStatuses.PENDING]: getTaskString.bind(null, 'taskWaitingStatus'),


This does indeed work, but I wonder if just an inline anonymous arrow function might be more comprehensible?

() => getTaskString('taskWaitingStatus')

🤦 good call :)

rtibbles · 2022-06-28T21:25:46Z

kolibri/plugins/device/assets/src/views/FacilitiesPage/FacilitiesTasksPage.vue

@@ -39,13 +39,13 @@

 <script>

+  import { FacilityTaskPanel } from 'kolibri.coreVue.componentSets.sync';


How do you feel about experimenting with putting some common code (but not really core code) into a module inside packages/kolibri-common or some such?

Should then be referenceable as kolibri-common/path/to/component.

If that would help lighten our default front-end bundle load at all with tree shaking, then I think it's a good call for sure. Otherwise I'd be hesitant to separate anything from the apiSpec since, in spite of it being a big monolith, is an easy thing to reason about and change - at least it is once it's clear the object maps to the aliased paths in imports.

Yes - I think a fair amount of the code that is in the default frontend bundle falls into this category, so if we can experiment with this here, it would set us up to migrate more things out of the frontend bundle, and reduce the size considerably, I think.

nucleogenesis · 2022-08-04T20:58:56Z

@radinamatic I've addressed the comments above.

Note that the solution to wrapping the radio button label requires overriding a default setting in KDS that KRadioButtons be truncated with ellipsis.

@rtibbles all feedback is addressed - I went to address your comment re: the exporting the translator method along with the mixins and create an issue bit figured it'd be just about as quick to just do some of the work up front here #9588

Also, created new issue re: a common module here #9589

radinamatic · 2022-08-08T06:45:03Z

Both missed focus traps are now working well, and the rest of my comments addressed! 👏🏽

Note that the solution to wrapping the radio button label requires overriding a default setting in KDS that KRadioButtons be
truncated with ellipsis.

If I understood @marcellamaki correctly, we are going to address that in KDS before 0.16 is released.

I'll wait for @pcenov to finish his round of testing to approve.

pcenov · 2022-08-08T12:02:00Z

Here is what I was able to identify as possible issues:

Issue	Screenshot
Select facility - 1. Which is the final label? 2. The 'Import learning facility - 1 of X' text is missing.
:-------------------------:	:-------------------------:
Import learning facility - Missing device name, network and facility name values
:-------------------------:	:-------------------------:
Import learning facility - Missing facility name
:-------------------------:	:-------------------------:

radinamatic · 2022-08-08T21:51:06Z

Thank you @pcenov, I was mostly concentrated on keyboard navigation, and missed to report these string discrepancies.

@nucleogenesis, note that all instances of the string facility in the setup wizard are now learning facility to give better context for localization.

I can also replicate the rest of the issues reported above: missing setup step, device name, string change in the third screenshot (learning facility is added) plus the facility name is not displayed.

…ity & Device. Mitigate issues with calling a translator before i18n is ready by partially applying the t8n fns to defer evaluation

…TaskUtils; remove dupe defs from device & facility constants.js

…ntication, handle clicks for data sync on evts, pass footer message to components

…to import from

…nt on continue, wrap in OnbStepBase

… FacilityTaskPanel

…t is a form element

…nue buttons (ie, during import loading)

…d to avoid mid-import nav

… remove "account" from end

… devtools warning

…is decided

…m on a kmodal

…line being clipped issue

…ssing in sync componentset

nucleogenesis · 2022-08-10T21:55:16Z

@radinamatic @pcenov

re: KRadioButton - You're right, this will be dealt with in KDS learningequality/kolibri-design-system#302

re: "Import learning facility - Missing device name, network and facility name values" - There are two forms of this message - the one shown on my side is for when there are multiple facilities so the facility name is mentioned in the message there. The one @pcenov shared is for when there is one facility on the device.

re: Import learning facility - Missing device name, network and facility name values - fixed in ad797f8

re: string updates
@radinamatic can you make an issue re: the string updates. There are a lot of uses of the word "facility" - so it may be a bit of work to do that might be a good issue to introduce people to strings later on.

pcenov · 2022-08-15T10:04:15Z

@radinamatic The following issues are still present in: https://buildkite.com/learningequality/kolibri-debian/builds/4738:

Issue	Screenshot
Select facility - 1. Which is the final label? 2. The 'Import learning facility - 1 of X' text is missing.
:-------------------------:	:-------------------------:
Import learning facility - Missing device name, network and facility name values
:-------------------------:	:-------------------------:

The facility name is displayed correctly now at the last step of 'Import learning facility, however the 'is' is missing:

@nucleogenesis Will those be fixed here or should I report them as separate issues?

radinamatic · 2022-08-15T21:55:09Z

We agreed with @nucleogenesis to open follow up issue to review the strings, so this should be good to go! 🎉

radinamatic

Manual QA is looking good! 💯

nucleogenesis changed the title ~~Onboarding import facility~~ Onboarding import facility - Mostly designs Jun 24, 2022

nucleogenesis requested review from radinamatic and marcellamaki June 24, 2022 18:35

nucleogenesis added the TODO: needs review Waiting for review label Jun 24, 2022

nucleogenesis added this to the 0.16.0 milestone Jun 24, 2022

nucleogenesis marked this pull request as ready for review June 24, 2022 19:29

nucleogenesis force-pushed the onboarding-import-facility branch from fdd325c to 60dd14c Compare June 24, 2022 19:30

nucleogenesis requested a review from rtibbles June 27, 2022 22:22

marcellamaki reviewed Jun 28, 2022

View reviewed changes

rtibbles reviewed Jun 28, 2022

View reviewed changes

nucleogenesis force-pushed the onboarding-import-facility branch from 605a14a to 5640e81 Compare August 4, 2022 17:23

This was referenced Aug 4, 2022

Align common string modules #9588

Merged

Research benefits & LOE of moving common code to package #9589

Closed

nucleogenesis added the TODO: needs QA re-review For stale issues that need to be revisited label Aug 4, 2022

nucleogenesis force-pushed the onboarding-import-facility branch from 80bd8c7 to 361d502 Compare August 10, 2022 20:33

nucleogenesis added 6 commits August 10, 2022 14:29

Move syncTaskUtils into core as it's shared across SetupWizard, Facil…

b3d2542

…ity & Device. Mitigate issues with calling a translator before i18n is ready by partially applying the t8n fns to defer evaluation

Import TaskTypes, TaskStatuses, TransferTypes from kolibri.utils.sync…

cb2b2c1

…TaskUtils; remove dupe defs from device & facility constants.js

Move FacilityTaskPanel(Details) to core & syncComponentSet

b215043

update selectFacilityTitle message to "Select learning facility"

2c5d6f2

Preclude non-Boolean return type on SelectAddressForm#submitDisabled

a5c9f68

Only render description prop <p> when there is a description to show

f53553b

nucleogenesis added 17 commits August 10, 2022 14:29

ImportFacilitySetup: remove styling, add step & component ImportAuthe…

0f5ae0d

…ntication, handle clicks for data sync on evts, pass footer message to components

ensure network address is selected before going to select a facility …

1e736b2

…to import from

SelectFacilityForm: remove all admin form business, emit data to pare…

ac156cb

…nt on continue, wrap in OnbStepBase

Wrap LoadingTaskPage with OnbStepBase, update title message, use core…

5fad68d

… FacilityTaskPanel

OnboardingStepBase listens for enter key, continues if focused elemen…

3fc56c0

…t is a form element

OnboardingStepBase navDisabled prop to permit disabling of back/conti…

479dacc

…nue buttons (ie, during import loading)

LoadingTaskPage disables nav when task is neither failed nor complete…

b883396

…d to avoid mid-import nav

SuperUserCredentialsForm header msg changed to "Create super admin" -…

4111c6b

… remove "account" from end

Ensure FacilityAdminCredentialsForm wrapped in <form> tags; remediate…

9c0a906

… devtools warning

skip LoadingTaskPage tests re: buttons - will update suite when flow …

9deda6e

…is decided

arrow func rather than bind

df54943

trap focus in language switcher modal

41bcada

focus doesn't get trapped when v-show already rendered it into the do…

dfaa167

…m on a kmodal

add new address button moved under address list, also fixes focus out…

dfc402e

…line being clipped issue

use the new getTaskString export

31051ce

ensure function not called until expected in sync status info, fix mi…

95afde2

…ssing in sync componentset

fix broken refs after rebase

6356f39

nucleogenesis force-pushed the onboarding-import-facility branch from 361d502 to 6356f39 Compare August 10, 2022 21:29

use extra_metadata field for facility name on task status string

ad797f8

nucleogenesis modified the milestones: 0.16.0, upcoming patch Aug 15, 2022

radinamatic approved these changes Aug 15, 2022

View reviewed changes

nucleogenesis merged commit 87f12da into learningequality:develop Aug 15, 2022

radinamatic mentioned this pull request Aug 16, 2022

Check 0.16 onboarding strings #9626

Closed

jredrejo mentioned this pull request Dec 21, 2022

Restore task types that had been deleted #9938

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Onboarding import facility - Mostly designs #9534

Onboarding import facility - Mostly designs #9534

nucleogenesis commented Jun 23, 2022 •

edited

Loading

github-actions bot commented Jun 23, 2022 •

edited

Loading

radinamatic commented Jun 28, 2022 •

edited

Loading

radinamatic commented Jun 28, 2022

marcellamaki commented Jun 28, 2022 •

edited

Loading

marcellamaki left a comment •

edited

Loading

marcellamaki Jun 28, 2022

nucleogenesis Jun 28, 2022

marcellamaki Jun 28, 2022

marcellamaki Jun 28, 2022 •

edited

Loading

jtamiace commented Jun 28, 2022

rtibbles left a comment

rtibbles Jun 28, 2022

nucleogenesis Jun 28, 2022

rtibbles Jun 28, 2022

rtibbles Jun 28, 2022

nucleogenesis Jun 28, 2022

rtibbles Jun 28, 2022

nucleogenesis Jun 28, 2022

rtibbles Jun 28, 2022

nucleogenesis commented Aug 4, 2022

radinamatic commented Aug 8, 2022

pcenov commented Aug 8, 2022 •

edited

Loading

radinamatic commented Aug 8, 2022 •

edited

Loading

nucleogenesis commented Aug 10, 2022

pcenov commented Aug 15, 2022

radinamatic commented Aug 15, 2022

radinamatic left a comment

		@@ -39,13 +39,13 @@

		<script>

		import { FacilityTaskPanel } from 'kolibri.coreVue.componentSets.sync';

Onboarding import facility - Mostly designs #9534

Onboarding import facility - Mostly designs #9534

Conversation

nucleogenesis commented Jun 23, 2022 • edited Loading

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Jun 23, 2022 • edited Loading

radinamatic commented Jun 28, 2022 • edited Loading

radinamatic commented Jun 28, 2022

marcellamaki commented Jun 28, 2022 • edited Loading

marcellamaki left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcellamaki Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

jtamiace commented Jun 28, 2022

rtibbles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nucleogenesis commented Aug 4, 2022

radinamatic commented Aug 8, 2022

pcenov commented Aug 8, 2022 • edited Loading

radinamatic commented Aug 8, 2022 • edited Loading

nucleogenesis commented Aug 10, 2022

pcenov commented Aug 15, 2022

radinamatic commented Aug 15, 2022

radinamatic left a comment

Choose a reason for hiding this comment

nucleogenesis commented Jun 23, 2022 •

edited

Loading

github-actions bot commented Jun 23, 2022 •

edited

Loading

radinamatic commented Jun 28, 2022 •

edited

Loading

marcellamaki commented Jun 28, 2022 •

edited

Loading

marcellamaki left a comment •

edited

Loading

marcellamaki Jun 28, 2022 •

edited

Loading

pcenov commented Aug 8, 2022 •

edited

Loading

radinamatic commented Aug 8, 2022 •

edited

Loading