(fix) O3-3890 Help menu (and menu items) should use a standard Carbon… #1164

Samstar10 · 2024-09-26T13:44:01Z

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

My work conforms to the O3 Styleguide and design documentation.

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

I have updated the help menu to use standard carbon menu items. This PR is related to this PR openmrs/openmrs-esm-user-onboarding#21

Screenshots

Related Issue

https://openmrs.atlassian.net/browse/O3-3890

Other

… menu

denniskigen · 2024-10-05T08:49:14Z

packages/apps/esm-help-menu-app/src/help-menu/components/docs.component.tsx

-      {t('docs', 'Docs')}
-    </Link>
+      label={t('documentation', 'Documentation')}
+      onClick={() => window.open('https://o3-docs.openmrs.org', '_blank')}


Can we move the onClick handler to a function that's wrapped in a useCallback hook?

denniskigen · 2024-10-05T08:58:04Z

...esm-primary-navigation-app/src/components/change-language/change-language-link.extension.tsx

I wouldn’t expect these changes to be included here. Bad rebase?

Yes, a mistake on my end, I'll fix it

denniskigen · 2024-10-05T09:09:02Z

packages/apps/esm-help-menu-app/src/help-menu/components/contact-us.component.tsx

-      {t('communityforum', 'Community forum')}
-    </Link>
+      label={t('communityforum', 'Community forum')}
+      onClick={() => window.open('https://talk.openmrs.org', '_blank')}


Can we wrap the onClick handlers in use callback hooks? Additionally, for security, it’s good to add noreferrer noopener to external links:

const handleClick = useCallback(() => { window.open('https://talk.openmrs.org', '_blank', 'noopener,noreferrer'); }, [url]);

denniskigen · 2024-10-06T19:33:28Z

One challenge with this approach is that we can't leverage the renderIcon prop of the MenuItem component. To leverage it, the MenuItem must have a Menu component as its direct parent, and Menu's mode prop should be set to basic.

brandones · 2024-10-21T19:03:43Z

@Samstar10 Are you planning on continuing work on this? When? Is it clear to you how to proceed?

Samstar10 · 2024-10-22T07:13:08Z

@Samstar10 Are you planning on continuing work on this? When? Is it clear to you how to proceed?

Hello @brandones , yes, I have already completed it actually, just having some troubles with pushing it, hoping to be helped during today's coffee break.

denniskigen · 2024-10-22T12:24:50Z

Mind resolving the conflicts, @Samstar10?

Samstar10 · 2024-10-22T12:26:21Z

Mind solving the conflicts, @Samstar10?

Yes, currently working on that.

denniskigen · 2024-10-22T12:28:15Z

packages/apps/esm-help-menu-app/src/help-menu/components/contact-us.component.tsx

 import { Link } from '@carbon/react';

 const ContactUs = () => {
  const { t } = useTranslation();
  return (
    <Link
      className={styles.helpButton}
+      style={{ textDecoration: 'none' }}


This should move to styles.scss.

@denniskigen for some reason whenever I move this to the styles.css it does not work. This is the only way it works.

denniskigen · 2024-10-22T12:28:32Z

packages/apps/esm-help-menu-app/src/help-menu/components/docs.component.tsx

 import { Link } from '@carbon/react';

 const Docs = () => {
  const { t } = useTranslation();
  return (
    <Link
      className={styles.helpButton}
+      style={{ textDecoration: 'none' }}


denniskigen · 2024-10-22T12:28:37Z

packages/apps/esm-help-menu-app/src/help-menu/components/release-notes.component.tsx

@@ -10,6 +10,7 @@ const ReleaseNotes = () => {
  return (
    <Link
      className={styles.helpButton}
+      style={{ textDecoration: 'none' }}


denniskigen · 2024-10-22T12:31:20Z

packages/apps/esm-help-menu-app/src/help-menu/components/styles.scss

  color: black !important;
  &:focus {
    outline: none;
  }
 }
+
+.helpButton:hover {
+  background: #e8e8e8;


Suggested change

background: #e8e8e8;

background: colors.$white-hover;

https://carbon-elements.netlify.app/colors/examples/preview/

@denniskigen this variable doesn't seem to exist

You can use colors.$gray-10-hover instead then:

denniskigen · 2024-10-22T12:32:21Z

packages/apps/esm-help-menu-app/src/help-menu/components/styles.scss

@@ -6,8 +6,13 @@
  display: flex;
  align-items: center;
  justify-content: space-between;
+  padding: 0.5rem 0.5rem;


Suggested change

padding: 0.5rem 0.5rem;

padding: layout.$spacing-03;

nit: padding: layout.spacing-03 is shorthand for padding-top: 0.5rem; padding-right: 0.5rem; padding-bottom: 0.5rem; padding-left: 0.5rem;

denniskigen · 2024-10-22T12:34:42Z

...esm-primary-navigation-app/src/components/change-language/change-language-link.extension.tsx

Why are we deleting this file? Seems unrelated to whatever else we're doing.

I was trying to sort out the bad rebase. Working on fixing it now.

* (fix) O3-3891: Adjust Change Language Modal Size to Small * (fix) O3-3891: Adjust Change Language Modal Size to Small * (fix) O3-3891: Adjust Change Language Modal Size to Small * (fix) O3-3891: Adjust Change Language Modal Size to Small

…eguide (openmrs#1163) * (feat) add checkmark--filled and information icons * (chore) update docs

Co-authored-by: OpenMRS Bot <[email protected]>

* Pass patient object to the banner action buttons * Update documentation

* Add nav group extension * Update docs * clean up * Remove unnecessary components * update docs * clean up * update t * define feature name for nav group * update docs * make group title translatable * update docs * tweak title translation and slot name * update api docs * register the global nav group extension * update docs * clean up * PR reviews * Update docs * Move nav group to primary-navigation-app --------- Co-authored-by: Ian <[email protected]>

Co-authored-by: OpenMRS Bot <[email protected]>

…nmrs#1166) * (chore) add checkmark--filled and information icons mocks * (fix) fix CheckmarkFilledIcon mock name

… component be reuseable (openmrs#1159)

This PR refactors some calls to the REST API to reuse the `restBaseUrl` variable.

…Icon

… functions (openmrs#1174)

…the LogIn page below the "Powered by" (openmrs#1127)

Co-authored-by: OpenMRS Bot <[email protected]>

* Add hover state to help menu and a hand pointer to the cursor * Minor tweaks --------- Co-authored-by: Dennis Kigen <[email protected]>

…#1181) * (chore) O3-4011 - add mock for PatientBannerPatientIdentifier * fix copy-paste typo

brandones · 2024-11-03T23:57:21Z

Oh boy. Can we get this cleaned up and in? Thank you for your help @ibacher

brandones · 2024-11-18T21:08:36Z

Hello, @Samstar10 , are you still working on this?

Samstar10 added 5 commits September 24, 2024 15:28

(fix) O3-3891: Adjust Change Language Modal Size to Small

c470659

(fix) O3-3891: Adjust Change Language Modal Size to Small

675378a

(fix) O3-3891: Adjust Change Language Modal Size to Small

700b47b

(fix) O3-3891: Adjust Change Language Modal Size to Small

26d30e5

(fix) O3-3890 Help menu (and menu items) should use a standard Carbon…

1df72a9

… menu

Samstar10 mentioned this pull request Sep 26, 2024

(fix) O3-3890 Help menu (and menu items) should use a standard Carbon… openmrs/openmrs-esm-user-onboarding#21

Open

3 tasks

denniskigen reviewed Oct 5, 2024

View reviewed changes

(fix) O3-3890 Help menu should use carbon design standards

2a25a9d

(fix) remove the change language link file.

cc86b33

denniskigen reviewed Oct 22, 2024

View reviewed changes

Samstar10 and others added 14 commits October 22, 2024 16:04

(feat) O3-4018: Add Information and checkmarkFilled icons to the styl…

a69def6

…eguide (openmrs#1163) * (feat) add checkmark--filled and information icons * (chore) update docs

(chore) Update translations from Transifex (openmrs#1086)

83bd341

Co-authored-by: OpenMRS Bot <[email protected]>

(fix) Fix merging configuration files to properly call readFile

41bb523

(fix) Pass patient object to the banner action buttons (openmrs#1158)

8c36d9f

* Pass patient object to the banner action buttons * Update documentation

(chore) Update translations from Transifex (openmrs#1165)

64abbff

Co-authored-by: OpenMRS Bot <[email protected]>

(chore) Update translations from Transifex (openmrs#1167)

42e3b62

Co-authored-by: OpenMRS Bot <[email protected]>

(chore) O3-4018: Add checkmarkFilled and information icons mocks (ope…

30c81fe

…nmrs#1166) * (chore) add checkmark--filled and information icons mocks * (fix) fix CheckmarkFilledIcon mock name

(chore) O3-4011 - refactor patient banner to have patient identifiers…

efb7351

… component be reuseable (openmrs#1159)

(refactor) Refactor REST API calls to reuse restBaseUrl (openmrs#1168)

1846ee0

This PR refactors some calls to the REST API to reuse the `restBaseUrl` variable.

fix CheckmarkFilled icon registration (openmrs#1170)

6946eae

(fix) fix checkmark filled icon (openmrs#1171)

fe432a0

(fix) fix checkmark filled icon (openmrs#1172)

15ccea3

ibacher and others added 18 commits October 22, 2024 16:04

(fix) Translation overrides should only apply to the specified language

0c5ed99

(fix) O3-4018: Fix checkmarkFilled icon and remove broken information…

40879f5

…Icon

(fix) O3-4018: Add fixed Information icon set

8034703

(fix) O3-4018: Add icon docs

c002d8a

(feat) Add variable name extractor to expression runner (openmrs#1173)

798f29a

(chore) Update TX configuration for merged translations\n

2472b81

(chore) Update translations from Transifex (openmrs#1169)

1305ecf

(BREAKING) Retire concept of ConnectedExtension (openmrs#1154)

7fbd81f

(fix) Expression evaluator should use typeof not instanceof to detect…

03c4540

… functions (openmrs#1174)

(fix) useAssignedExtensions should not return undefined

9dc2ada

(feat) add CheckmarkOutlineIcon (openmrs#1176)

84c2bf7

(feature) O3-3859 Ability to customize or add logos at the bottom of …

d6eb647

…the LogIn page below the "Powered by" (openmrs#1127)

(chore) Update translations from Transifex (openmrs#1175)

5bc90c2

Co-authored-by: OpenMRS Bot <[email protected]>

(fix) Fix docstring for extractVariableNames (openmrs#1178)

071d83f

(fix) O3-3888: Tweak help menu hover state (openmrs#1179)

f3dd5de

* Add hover state to help menu and a hand pointer to the cursor * Minor tweaks --------- Co-authored-by: Dennis Kigen <[email protected]>

(test) O3-4011 - add mock for PatientBannerPatientIdentifier (openmrs…

bd6f8ec

…#1181) * (chore) O3-4011 - add mock for PatientBannerPatientIdentifier * fix copy-paste typo

(fix) O3-3891: Adjust Change Language Modal Size to Small

3d49273

(fix) O3-3890 Help menu should use carbon design standards

9122f78

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(fix) O3-3890 Help menu (and menu items) should use a standard Carbon… #1164

(fix) O3-3890 Help menu (and menu items) should use a standard Carbon… #1164

Samstar10 commented Sep 26, 2024 •

edited by denniskigen

Loading

denniskigen Oct 5, 2024

Samstar10 Oct 7, 2024

denniskigen Oct 5, 2024

Samstar10 Oct 7, 2024

denniskigen Oct 5, 2024 •

edited

Loading

Samstar10 Oct 7, 2024

denniskigen commented Oct 6, 2024

brandones commented Oct 21, 2024

Samstar10 commented Oct 22, 2024

denniskigen commented Oct 22, 2024 •

edited

Loading

Samstar10 commented Oct 22, 2024

denniskigen Oct 22, 2024

Samstar10 Oct 22, 2024

denniskigen Oct 22, 2024

denniskigen Oct 22, 2024

denniskigen Oct 22, 2024

Samstar10 Oct 22, 2024

denniskigen Oct 22, 2024

denniskigen Oct 22, 2024

Samstar10 Oct 22, 2024

denniskigen Oct 22, 2024

Samstar10 Oct 22, 2024

brandones commented Nov 3, 2024

brandones commented Nov 18, 2024

(fix) O3-3890 Help menu (and menu items) should use a standard Carbon… #1164

Are you sure you want to change the base?

(fix) O3-3890 Help menu (and menu items) should use a standard Carbon… #1164

Conversation

Samstar10 commented Sep 26, 2024 • edited by denniskigen Loading

Requirements

For changes to apps

If applicable

Summary

Screenshots

Related Issue

Other

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

denniskigen Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniskigen commented Oct 6, 2024

brandones commented Oct 21, 2024

Samstar10 commented Oct 22, 2024

denniskigen commented Oct 22, 2024 • edited Loading

Samstar10 commented Oct 22, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandones commented Nov 3, 2024

brandones commented Nov 18, 2024

Samstar10 commented Sep 26, 2024 •

edited by denniskigen

Loading

denniskigen Oct 5, 2024 •

edited

Loading

denniskigen commented Oct 22, 2024 •

edited

Loading