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

(feat) O3-3390: Implement the help menu #1034

Merged
merged 21 commits into from
Jul 2, 2024

Conversation

Vijaykv5
Copy link
Contributor

@Vijaykv5 Vijaykv5 commented Jun 8, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as feat, fix, or chore, among others). See existing PR titles for inspiration.

For changes to apps

If applicable

  • My work includes tests or is validated by existing tests.
  • I have updated the esm-framework mock to reflect any API changes I have made.

Summary

  • This PR implements the help menu along with it's submenu in a new module named esm-help-menu-app.
  • the submenu consists of walkthroughs and view tutorials, contact us, release notes and docs.

Screenshots

help-menu.mov

Related Issue

O3-3390

Other

The help menu is design as per these Zeplin Designs

@Vijaykv5
Copy link
Contributor Author

Vijaykv5 commented Jun 8, 2024

@Piumal1999 @jayasanka-sack @ibacher @denniskigen seeking you review
Thanks!

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

A couple of broader things:

  • Ideally, this would be located where the devtools button is. Maybe we move the devtools.
  • I'm not a massive fan of the modal side menu. I think a larger modal in the middle of the screen makes more sense.
  • The release notes should be an accordion that expands to show the notes.

@Vijaykv5 Vijaykv5 marked this pull request as draft June 14, 2024 15:40
Comment on lines 28 to 45
<button className={styles.helpButton} onClick={props.close}>
{t('releaseNotes', 'Release Notes')}
</button>
<button className={styles.helpButton} onClick={handleTutorialsModalOpen}>
{t('tutorials', 'Tutorials')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('contactUs', 'Contact us')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('docs', 'Docs')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('feedbackForum', 'Feedback Forum')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('version', 'OpenMRS 3.0.1')}
</button>
Copy link
Member

Choose a reason for hiding this comment

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

@Vijaykv5 The extension slot should come here, right? Not the help.component.tsx

Suggested change
<button className={styles.helpButton} onClick={props.close}>
{t('releaseNotes', 'Release Notes')}
</button>
<button className={styles.helpButton} onClick={handleTutorialsModalOpen}>
{t('tutorials', 'Tutorials')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('contactUs', 'Contact us')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('docs', 'Docs')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('feedbackForum', 'Feedback Forum')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('version', 'OpenMRS 3.0.1')}
</button>
<ExtensionSlot className={styles.helpMenuExtension} name="help-menu-slot"/>

Copy link
Contributor Author

@Vijaykv5 Vijaykv5 Jun 18, 2024

Choose a reason for hiding this comment

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

yeah that's right!
I thought the help menu button should also have to be an extension.
Thanks @jayasanka-sack

Copy link
Contributor

Choose a reason for hiding this comment

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

@jayasanka-sack, just to clarify, we will be using extension slots in two scenarios, right?

  1. To get the list of items in the help menu
  2. To load the tutorials list

Copy link
Contributor

@Piumal1999 Piumal1999 Jun 19, 2024

Choose a reason for hiding this comment

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

By the way, if we do the suggested change now, we will have to create an extension for the help-menu-slot that has an entry point to packages/apps/esm-help-menu-app/src/helpMenu/subHelpMenu/tutorial.tsx component. Otherwise, there won’t be a way to open the tutorial modal.

Copy link
Member

Choose a reason for hiding this comment

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

just to clarify, we will be using extension slots in two scenarios, right?

@Piumal1999 yes

Comment on lines 28 to 45
<button className={styles.helpButton} onClick={props.close}>
{t('releaseNotes', 'Release Notes')}
</button>
<button className={styles.helpButton} onClick={handleTutorialsModalOpen}>
{t('tutorials', 'Tutorials')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('contactUs', 'Contact us')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('docs', 'Docs')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('feedbackForum', 'Feedback Forum')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('version', 'OpenMRS 3.0.1')}
</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

@jayasanka-sack, just to clarify, we will be using extension slots in two scenarios, right?

  1. To get the list of items in the help menu
  2. To load the tutorials list

Comment on lines 28 to 45
<button className={styles.helpButton} onClick={props.close}>
{t('releaseNotes', 'Release Notes')}
</button>
<button className={styles.helpButton} onClick={handleTutorialsModalOpen}>
{t('tutorials', 'Tutorials')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('contactUs', 'Contact us')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('docs', 'Docs')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('feedbackForum', 'Feedback Forum')}
</button>
<button className={styles.helpButton} onClick={props.close}>
{t('version', 'OpenMRS 3.0.1')}
</button>
Copy link
Contributor

@Piumal1999 Piumal1999 Jun 19, 2024

Choose a reason for hiding this comment

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

By the way, if we do the suggested change now, we will have to create an extension for the help-menu-slot that has an entry point to packages/apps/esm-help-menu-app/src/helpMenu/subHelpMenu/tutorial.tsx component. Otherwise, there won’t be a way to open the tutorial modal.

@Vijaykv5
Copy link
Contributor Author

Vijaykv5 commented Jun 20, 2024

The updated changes looks like this
As we discussed the tutorials would be another extension, so I moved that from this PR.
I also couldn't figure out for feedback forum and OpenMRS 3.0.1 field as sub menu items

helpmenu-new.mov

cc : @jayasanka-sack @Piumal1999

Copy link
Contributor

@Piumal1999 Piumal1999 left a comment

Choose a reason for hiding this comment

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

Good job @Vijaykv5. I requested few minor changes.

Btw, for the contact us, docs, and release notes buttons, we could use a single reusable component, as all three have the same structure, styles, and behavior. But we can do it later as this PR needs to be merged as soon as possible in order to continue the project.

@Vijaykv5 Vijaykv5 requested a review from Piumal1999 June 24, 2024 06:46

useEffect(() => {
const handleClickOutside = (event) => {
if (!event.target.closest(`.${styles.helpMenuButton}`) && !event.target.closest(`.${styles.popup}`)) {
Copy link
Contributor

@Piumal1999 Piumal1999 Jun 24, 2024

Choose a reason for hiding this comment

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

It would be better if we could identify the event in some other way than using styles. maybe with react 'ref's

Copy link
Contributor

@Piumal1999 Piumal1999 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@Vijaykv5 Vijaykv5 marked this pull request as ready for review June 24, 2024 17:50
@Vijaykv5 Vijaykv5 requested a review from Piumal1999 June 24, 2024 17:50
@gracepotma
Copy link
Contributor

Could we change "Contact us" to "Community Forum" and link that to talk.openmrs.org ?

@jayasanka-sack
Copy link
Member

Latest screenshot:

image

@jayasanka-sack jayasanka-sack changed the title (feat) O3-3390 : Implement Help Menu for User Onboardings (feat) O3-3390 : Implement the help menu Jun 26, 2024
@jayasanka-sack
Copy link
Member

jayasanka-sack commented Jun 26, 2024

  • Ideally, this would be located where the devtools button is. Maybe we move the devtools.
  • I'm not a massive fan of the modal side menu. I think a larger modal in the middle of the screen makes more sense.
  • The release notes should be an accordion that expands to show the notes.

@paulsonder @gracepotma do you have any input on this suggestion?

This is the provided design:
https://zpl.io/5E6PWgW

Regarding the the icon location, I'm all in with Ian's suggestion, but I'm not sure where to move the devtools icon. Any ideas?

@jayasanka-sack
Copy link
Member

@Vijaykv5 could you please check why the CI build is failing?

Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

Good start, @Vijaykv5. I've left some feedback.

"name": "@openmrs/esm-help-menu-app",
"version": "5.6.0",
"license": "MPL-2.0",
"description": " A microfrontend that provides a help menu for the OpenMRS Microfrontend Framework",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"description": " A microfrontend that provides a help menu for the OpenMRS Microfrontend Framework",
"description": " A microfrontend that provides a help menu for O3",

],
"noEmit": true
},
"include": ["src/**/*", "../esm-help-menu-app/src/help-menu copy"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"include": ["src/**/*", "../esm-help-menu-app/src/help-menu copy"]
"include": ["src/**/*"]

Why is the second bit necessary?

Comment on lines +1 to +3
(window as any).importMapOverrides = {
getOverrideMap: jest.fn().mockReturnValue({ imports: {} }),
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(window as any).importMapOverrides = {
getOverrideMap: jest.fn().mockReturnValue({ imports: {} }),
};
import '@testing-library/jest-dom';
(window as any).importMapOverrides = {
getOverrideMap: jest.fn().mockReturnValue({ imports: {} }),
};

],
"extensions": [
{
"name": "release-note",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"name": "release-note",
"name": "release-notes",

Comment on lines 3 to 5
import Release from './help-menu/components/release-notes.component';
import Docs from './help-menu/components/docs.component';
import contactus from './help-menu/components/contact-us.component';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import Release from './help-menu/components/release-notes.component';
import Docs from './help-menu/components/docs.component';
import contactus from './help-menu/components/contact-us.component';
import ReleaseNotesComponent from './help-menu/components/release-notes.component';
import DocsComponent from './help-menu/components/docs.component';
import ContactUsComponent from './help-menu/components/contact-us.component';

Comment on lines 41 to 48
<div role="button" onClick={toggleHelpMenu} ref={helpMenuButtonRef} className={classNames(styles.helpMenuButton)}>
<Help size={24} />
</div>
{helpMenuOpen && (
<div ref={popupRef}>
<HelpMenuPopup />
</div>
)}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div role="button" onClick={toggleHelpMenu} ref={helpMenuButtonRef} className={classNames(styles.helpMenuButton)}>
<Help size={24} />
</div>
{helpMenuOpen && (
<div ref={popupRef}>
<HelpMenuPopup />
</div>
)}
<button
aria-expanded={helpMenuOpen}
aria-controls="help-menu-popup"
onClick={toggleHelpMenu}
ref={helpMenuButtonRef}
className={classNames(styles.helpMenuButton)}
>
<Help size={24} />
</button>
{helpMenuOpen && (
<div id="help-menu-popup" ref={popupRef} className={styles.helpMenuPopup}>
<HelpMenuPopup />
</div>
)}

Copy link
Member

Choose a reason for hiding this comment

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

This file should have a .scss extension.

Comment on lines 2 to 12
z-index: 7900;
background-color: #c6c6c6;
height: 2.5rem !important;
width: 2.5rem !important;
bottom: 5rem;
position: fixed;
display: flex;
justify-content: center;
align-items: center;
right: 0.5rem;
border-radius: 2rem;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
z-index: 7900;
background-color: #c6c6c6;
height: 2.5rem !important;
width: 2.5rem !important;
bottom: 5rem;
position: fixed;
display: flex;
justify-content: center;
align-items: center;
right: 0.5rem;
border-radius: 2rem;
z-index: 7900;
background-color: white;
height: 2.5rem !important;
width: 2.5rem !important;
bottom: 5rem;
position: fixed;
display: flex;
justify-content: center;
align-items: center;
right: 0.5rem;
border: none;
box-shadow:
0 1px 3px rgba(0, 0, 0, 0.1),
0 1px 2px -1px rgba(0, 0, 0, 0.1);
border-radius: 2rem;

Copy link
Member

Choose a reason for hiding this comment

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

To get the button aligned with https://zpl.io/5E6PWgW.

Copy link
Member

Choose a reason for hiding this comment

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

Just realized the link I posted might not be the latest available design. If not, feel free to ignore the background color change suggestion above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's the latest design @denniskigen
Thank you :)

@@ -0,0 +1,21 @@
module.exports = {
transform: {
'\\.(m?j|t)sx?$': ['@swc/jest'],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'\\.(m?j|t)sx?$': ['@swc/jest'],
'^.+\\.tsx?$': ['@swc/jest'],

},
setupFiles: ['<rootDir>/src/setup-tests.ts'],
moduleNameMapper: {
'lodash-es': 'lodash',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'lodash-es': 'lodash',
'lodash-es': 'lodash',
'^lodash-es/(.*)$': 'lodash/$1',

@Vijaykv5 Vijaykv5 requested a review from denniskigen July 1, 2024 18:24
@Vijaykv5
Copy link
Contributor Author

Vijaykv5 commented Jul 1, 2024

Latest changes looks like this

help-menu.updated.mov

"@carbon/react": "1.37.0",
"fuzzy": "^0.1.3"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
}

@denniskigen denniskigen changed the title (feat) O3-3390 : Implement the help menu (feat) O3-3390: Implement the help menu Jul 2, 2024
Copy link
Member

@jayasanka-sack jayasanka-sack left a comment

Choose a reason for hiding this comment

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

Thanks @Vijaykv5 !

Merging this PR because the dev tools icon is only visible to admins, who rarely use the help functionality. However, we should discuss a proper decision in the next UX call.

@jayasanka-sack jayasanka-sack merged commit b230d99 into openmrs:main Jul 2, 2024
10 of 11 checks passed
@Vijaykv5 Vijaykv5 deleted the feat/esm-help branch July 2, 2024 15:21
Twiineenock pushed a commit to Twiineenock/openmrs-esm-core that referenced this pull request Jul 18, 2024
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.

6 participants