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-2948 Add frontend config to RefApp #787

Merged
merged 1 commit into from
Mar 15, 2024
Merged

(feat) O3-2948 Add frontend config to RefApp #787

merged 1 commit into from
Mar 15, 2024

Conversation

chibongho
Copy link
Contributor

As mentioned in O3-2948, we want to be able to configure for demo data in the frontend. This PR adds the ability to do that.

Tested as follows:

  • hard-code a different brand color in config-core_demo.json
  • do a docker-compose build then docker-compose up.
  • verify that a different brand color shows up when going to http://localhost

@@ -0,0 +1,5 @@
{
"@openmrs/esm-styleguide": {
"Brand color #1": "#005d5d"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The is the default brand color already. This entry isn't really needed, but is here for testing and as PoC.

Copy link
Member

@mseaton mseaton left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @chibongho . Interested in how @ibacher and others see this.

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.

Couple of quick things. Need to think about this a bit. I think, in an ideal world, we'd have this as something that is just part of a "standard" frontend build rather than a separate file added to the distro.

@@ -16,7 +16,7 @@ services:
environment:
SPA_PATH: /openmrs/spa
API_URL: /openmrs
SPA_CONFIG_URLS:
SPA_CONFIG_URLS: config-core_demo.json
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
SPA_CONFIG_URLS: config-core_demo.json
SPA_CONFIG_URLS: /openmrs/spa/config.json

Copy link
Member

Choose a reason for hiding this comment

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

Note I'm also renaming the file. I don't know that we need the same naming scheme here.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this file already relative to /openmrs/spa/ by default @ibacher ? Can't we just point this to "config.json" then?

@chibongho
Copy link
Contributor Author

@ibacher Do you have new thoughts on this? I'm happy to prototype something if you can suggest a different place to put this.

@ibacher
Copy link
Member

ibacher commented Mar 15, 2024

I actually thought this had been merged in. Might as well get it in for now and we can work on a slightly different approach later.

@ibacher ibacher merged commit 955c966 into main Mar 15, 2024
7 checks passed
@ibacher ibacher deleted the O3-2948 branch March 15, 2024 16:04
@pgorrindo
Copy link

Thanks @chibongho for your work on this feature!

For the life of me, however, I can't get it to work.

On a fresh clone of the openmrs-distro-referenceapplication repo, I've locally edited frontend/config-core_demo.json with

"@openmrs/esm-styleguide": {
    "Brand color #1": "#000000",
    "Brand color #2": "#000000",
    "Brand color #3": "#000000"
  }

(note the #000000 is just for trying to get this feature to work, I'd never actually make all the colors black intentionally)

and then several different tries of docker-compose build and docker-compose up and wiping my Docker containers and volumes for good measure and trying different browsers in case some styling were cached, etc -- but the styleguide-specified colors in config-core_demo.json aren't getting picked up. Instead, in the client, I see:

Screenshot 2024-04-21 at 18 30 30

Which is confusing, since the message on the right ("The current value comes from /openmrs/spa/config-core_demo.json") suggests my local config colors ought to be picked up from there -- but then clearly, they aren't (i.e., would expect the color value on the left to be #000000)

Any thoughts on what might be going on here?

@mseaton or @ibacher any thoughts from either of you on this, since you previously reviewed this PR?

Apologies if I'm misunderstanding something here. (Also apologies if you all would prefer I posted this at https://talk.openmrs.org -- since you have issues turned off for this repo.)

thanks for any thoughts or insight!

@mseaton
Copy link
Member

mseaton commented Apr 22, 2024

@pgorrindo - I'd have to see your entire setup to understand more. Are you sure you are actually running the docker container that you have built locally, and not running the default "qa" tag of the docker container? What do your docker-compose files look like and what command are you running to start the containers up?

@chibongho
Copy link
Contributor Author

@pgorrindo another thing I would try is to do do a hard refresh of the page (Ctrl+Shift+R). Also, you can do "view source" in the browser to view the HTML and see of the config is set correctly. There should be a relatively human-readable <script> tag with a configUrls defined in a JSON object. Make sure that it looks something like this:
configUrls: ["/openmrs/spa/config-core_demo.json"]

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.

4 participants