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

When accessing the Authn MFE first time, It shows "Authn | null" in the header #1158

Closed
hinakhadim opened this issue Feb 9, 2024 · 6 comments
Labels
bug Report of or fix for something that isn't working as intended

Comments

@hinakhadim
Copy link
Contributor

hinakhadim commented Feb 9, 2024

Whenever I clicked on 'Sign In' button, it takes me to the Authn MFE. The issue is that it shows 'Authn | null' in the header. The reason is that in public/index.html, the header is <title>Authn | <%= process.env.SITE_NAME %></title>. For first time, the process.env.SITE_NAME is null and after that mergeConfig runs which updates SITE_NAME. Due to that thing, it shows Authn | null which is not a good user experience. I will suggest to remove | <%= process.env.SITE_NAME %> and its better to just show Authn only in title. Here is the screenshot:

Screenshot 2024-02-09 at 10 12 25 AM

I am running openedx using tutor local

@zainab-amir zainab-amir added the bug Report of or fix for something that isn't working as intended label Feb 13, 2024
@regisb
Copy link
Contributor

regisb commented Feb 27, 2024

Hi Hina! 👋 Removing the entire | <%= process.env.SITE_NAME %> is going to be a breaking change for every Open edX platform out there, and a very debatable one.

Can I suggest instead to only remove the site name when it's null? That way, the title would be "Authn" while loading the page, and then become "Authn | site_name" once mergeConfig is run. Is that even possible?

A separate question would be to rename the "Authn" title to "Authentication". I think there's consensus to say that "Authn" makes for a terrible site name.

@attiyaIshaque
Copy link
Contributor

@hinakhadim The issue has been resolved in Pull Request #1168, and I am closing the PR with the resolution of "Fixed."

@attiyaIshaque
Copy link
Contributor

@regisb I have implemented this suggested change.

Can I suggest instead to only remove the site name when it's null? That way, the title would be "Authn" while loading the page, and then become "Authn | site_name" once mergeConfig is run. Is that even possible?

@zainab-amir
Copy link
Contributor

zainab-amir commented Feb 27, 2024

I think there's consensus to say that "Authn" makes for a terrible site name.

I agree. I'll confirm from product and then @attiyaIshaque lets create a follow up PR to do this too.

@regisb
Copy link
Contributor

regisb commented Feb 28, 2024

Oh wow. Simple and efficient, I love it! Feel free to open a similar PR to cherry-pick the change in the open-release/quince.master branch. That way we will benefit from this change the next time a minor release tag is created (should be some time around april, I believe).

@hinakhadim
Copy link
Contributor Author

Hi all, I have tested the fixed PR #1168 on my machine. It haven't resolved the issue for me. It treats null as string and pass the condition which should be failed. I have created another PR #1185. I kindly request your assistance in reviewing this latest submission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended
Projects
None yet
Development

No branches or pull requests

4 participants