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

Adds the possibility for GLPI users to discover features with short tutorials #15524

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ccailly
Copy link
Member

@ccailly ccailly commented Sep 1, 2023

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets

Adds a discovery system for GLPI functionalities.

image
image
image

@cedric-anne cedric-anne self-requested a review September 4, 2023 07:29
Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

I think some changes targeting the /locales directory were maybe commited by mistake ?

image

Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

Functional review :

  1. The default lesson should not be triggered on the install page
    image

  2. It would be nice if the last step of the default lesson would show you the menu entry to access all lessons.

  3. When a lesson is finished, maybe add a final step that confirm the lesson is over and display a link to go to the lessons page ?

  4. If I keep the main lesson page open in browser tab A, then complete one of the lessons in browser tab B and finally try to run the same lesson from tab A (where its still marked as "not done" because I haven't refreshed the page) I get a blank page.
    Maybe in this case allow the user to run the lesson again even if its finished to avoid the error ?

  5. I can still scroll on the page when the modals are displayed.
    It feels a bit weird and I'm not sure if it's supposed to work this way.
    Is it done on purpose by the intro.js lib and/or is there an option to disable scrolling ?
    For example bootstrap modals forbid body scrolling by default:
    image

  6. Lessons are still available for all users but you end up with a blank page when trying to start a lesson if you are not super-admin.
    I think we could disable the "My lessons" menu entry for non super-admin users for now ?

  7. UI / UX is very good 👍, I still have a few minimal ajustements in mind but I think its best we wait till all the features are implemented before nitpicking on small details ;)

ajax/discover.php Outdated Show resolved Hide resolved
ajax/discover.php Outdated Show resolved Hide resolved
css/standalone/introjs.scss Show resolved Hide resolved
ajax/discover.php Outdated Show resolved Hide resolved
ajax/discover.php Outdated Show resolved Hide resolved
src/Discover/Discover.php Outdated Show resolved Hide resolved
src/Discover/Discover.php Outdated Show resolved Hide resolved
src/Discover/Discover_User.php Outdated Show resolved Hide resolved
src/Discover/Discover_User.php Outdated Show resolved Hide resolved
src/Discover/Discover_User.php Outdated Show resolved Hide resolved
@ccailly ccailly force-pushed the feature/discover branch 4 times, most recently from 8801e0b to 0e44dd1 Compare October 12, 2023 07:41
@ccailly ccailly force-pushed the feature/discover branch 2 times, most recently from c7a4b71 to 3cf524a Compare October 26, 2023 09:28
@ccailly ccailly marked this pull request as ready for review October 26, 2023 11:44
@orthagh orthagh linked an issue Nov 14, 2023 that may be closed by this pull request
@orthagh orthagh removed this from the 10.1.0 milestone Nov 14, 2023
@AdrienClairembault AdrienClairembault requested review from orthagh and cedric-anne and removed request for cedric-anne November 14, 2023 09:32
@AdrienClairembault
Copy link
Contributor

AdrienClairembault commented Nov 14, 2023

Note: don't forget we need a temporary branch to merge into as we want to split the development into multiple PRs.

Can somebody with write permission create the branch so we can rebase the PR onto it ?

@ccailly ccailly force-pushed the feature/discover branch 2 times, most recently from 5ca3eed to 84fdb4b Compare November 23, 2023 14:45
Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

Lint is not OK, can you check when you come back @ccailly ?

@ccailly ccailly force-pushed the feature/discover branch 2 times, most recently from 0c665f7 to 346bc6e Compare December 22, 2023 15:14
src/Discover/Discover_User.php Outdated Show resolved Hide resolved
templates/discover/lessons.js.twig Outdated Show resolved Hide resolved
Copy link
Contributor

@orthagh orthagh left a comment

Choose a reason for hiding this comment

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

Tests of the days:

  • lessons repo was not up to date, error reported in logs:
[2024-01-29 14:56:56] glpiphplog.CRITICAL:   *** Uncaught Exception TypeError: array_walk_recursive(): Argument #1 ($array) must be of type array, int given in /var/www/html/glpi/main/src/Discover/Discover_User.php at line 146
  • After update, it still fails on others lessons on my side, with a log:
[2024-01-29 15:23:18] glpiphplog.WARNING:   *** PHP Warning (2): foreach() argument must be of type array|object, null given in /var/www/html/glpi/main/vendor/twig/twig/src/Extension/CoreExtension.php at line 1695
  Backtrace :
  ...ates/30/30c1f1e67a16ab7ba2732d3d9e17b1db.php:46 twig_array_map()
  vendor/twig/twig/src/Template.php:394              __TwigTemplate_7c863886c16976a0a8246c1a67f306db->doDisplay()
  vendor/twig/twig/src/Template.php:367              Twig\Template->displayWithErrorHandling()
  vendor/twig/twig/src/TemplateWrapper.php:45        Twig\Template->display()
  src/Application/View/TemplateRenderer.php:198      Twig\TemplateWrapper->display()
  src/Discover/Discover.php:75                       Glpi\Application\View\TemplateRenderer->display()
  src/Html.php:1789                                  Glpi\Discover\Discover::loadDiscover()
  front/ticket.php:63                                Html::footer()
  public/index.php:82                                require()

Don't know why the last appears, any hints welcomed.

Regarding the config file in lessons folder, I see this kind of path: "content" => "file://introduction.md",. I suggest to move to file://./sources/introduction.md to permits to click on links in vscode.

@ccailly ccailly changed the base branch from feature/discover to main February 1, 2024 11:03
@ccailly ccailly force-pushed the feature/discover branch 3 times, most recently from fd54918 to 5b332f8 Compare February 1, 2024 11:13
ajax/discover.php Outdated Show resolved Hide resolved
ajax/discover.php Outdated Show resolved Hide resolved
js/discover.js Outdated Show resolved Hide resolved
* ---------------------------------------------------------------------
*/

return [
Copy link
Contributor

Choose a reason for hiding this comment

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

If the lessons are going to be part of the GLPI repo, the translations would benefit from having a context specified.

Copy link
Contributor

@orthagh orthagh Feb 2, 2024

Choose a reason for hiding this comment

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

good point !

The current lessons are, for the moment, examples of what is possible with the feature.
I pushed the subject to @arthurrschaefer, he should start soon to write (in our outline) articles.

When we will integrate his work, indeed, we should add gettext context.

resources/Lessons/config.php Outdated Show resolved Hide resolved
resources/Lessons/sources/assistance/create-ticket.md Outdated Show resolved Hide resolved
resources/Lessons/sources/assistance/empty-ticket-page.md Outdated Show resolved Hide resolved
resources/Lessons/sources/assistance/ticket-dashboard.md Outdated Show resolved Hide resolved
resources/Lessons/sources/introduction.md Outdated Show resolved Hide resolved
templates/layout/parts/page_header.html.twig Outdated Show resolved Hide resolved
Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

Some lessons content can't be fetched if the locates aren't compiled.

image

I think in this case we should still be able to display the default (english) content.

Also the compile scripts doesn't seem to work on my side, can you test it again to make sure it is OK ?

@ccailly
Copy link
Member Author

ccailly commented Feb 6, 2024

Some lessons content can't be fetched if the locates aren't compiled.

image

I think in this case we should still be able to display the default (english) content.

Also the compile scripts doesn't seem to work on my side, can you test it again to make sure it is OK ?

I've added a fallback on the sources in case no translation exists.
To run the build script, you need to be in the resources/Lessons folder. This script is temporary and will disappear or be modified.

@AdrienClairembault
Copy link
Contributor

Any updates on this ? Some reviews are still pending.

@ccailly ccailly changed the title Adds the possibility for GLPi users to discover features with short tutorials Adds the possibility for GLPI users to discover features with short tutorials Aug 30, 2024
@ccailly ccailly force-pushed the feature/discover branch 2 times, most recently from e12db21 to 9626486 Compare September 10, 2024 08:31
js/modules/Discover.js Outdated Show resolved Hide resolved
js/modules/Discover.js Outdated Show resolved Hide resolved
ccailly and others added 6 commits September 19, 2024 10:23
…utorials

Revert composer.lock unexpected changes

fix: resolve promise immediately if dropdown menu is not animated

fix: assign returned config array to variable

refactor: move launchIntro function in a separate JS file

fix: JS lint

fix: 1 file without header

fix: update lessons repo version

fix: change default value of endingSteps to empty array

refactor: Move lessons in the main GLPI repository

refactor: simplify dropdown-show action data structure

refactor: update of markdown file sources to enable clicking on links in vscode

fix: CI

Apply suggestions from code review

Co-authored-by: Curtis Conard <[email protected]>
fix: fallback to the sources if the english translation doesn't exist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wizard - discover GLPI
5 participants