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

(OUI Next Theme) Log in #1529

Closed
2 of 3 tasks
Tracked by #895 ...
KrooshalUX opened this issue Jul 25, 2023 · 16 comments
Closed
2 of 3 tasks
Tracked by #895 ...

(OUI Next Theme) Log in #1529

KrooshalUX opened this issue Jul 25, 2023 · 16 comments
Assignees

Comments

@KrooshalUX
Copy link

KrooshalUX commented Jul 25, 2023

This issue to be transferred to corresponding repository

I am working on launching new light and dark mode themes provided by OUI component library for a target launch within 2.10. These changes support the vision expressed in Future Vision for Dashboards

I have identified the following front end related issues that prevent the theme from appearing complete and potential solutions within this feature:

  • Log in page should reflect theme set by admin (v7 light, v7 dark, next light, next dark)
  • When theme is set to Next Dark or v7 dark by admin, Auto-filled text appears illegible. Auto filled text should not receive a different styling treatment as manually entered text.
  • When theme is set to Next Dark or v7 dark by admin, OpenSearch logo should display dark mode variant (currently showing light mode variant in dark mode theme)
Screen Shot 2023-07-31 at 2 55 53 PM
@KrooshalUX
Copy link
Author

@wbeckler @mnkugler top discussion question for you - my recommendation would be to use the current default since we will not be defaulting to next dark mode in 2.10 any longer.

@wbeckler
Copy link

Let's use light mode for login, but why not "Next"?

@wbeckler
Copy link

Is login part of security dashboard plugin?

@KrooshalUX
Copy link
Author

@wbeckler "why not next" - until we cut over, it would be potentially very strange to see the new "primary" color and then land in an experience with a different "primary" color

re: security plugin - I am not sure @kgcreative do you know?

@kgcreative
Copy link
Member

Login is provided by the security dashboards plugin, yes. IIRC it's a standalone page since the application hasn't loaded at that point

@wbeckler wbeckler transferred this issue from opensearch-project/oui Jul 31, 2023
@stephen-crawford
Copy link
Contributor

[Triage] Hi @KrooshalUX, can you please provide more information about what actionable items you would like addressed? This is especially important if we want this for 2.10, but it is not clear what is expected on the Security repository's behalf. Thank you.

@AMoo-Miki
Copy link
Contributor

AMoo-Miki commented Jul 31, 2023

There are two main tasks:

  1. make sure the login pages uses uiSettings.getOverrideOrDefault('theme:version')) and uiSettings.getOverrideOrDefault('theme:darkMode')) if it is dependent on the theme in any way.
  2. make sure the imagery and any styling on the login page work in dark mode:
    • if using any SVGs, make sure they are color-scheme-aware like this.
    • if you have any styling, make sure they use $eui... colors or target dark-mode as well using @media (prefers-color-scheme: dark)

@davidlago
Copy link

@kamingleung can you confirm if this has been decided and we're good to go ahead?

@KrooshalUX
Copy link
Author

KrooshalUX commented Jul 31, 2023

For reference, the discussion above that I posed about defaults in 2.10 is irrelevant of the choices we make around defaults at time shipping - because once an admin changes their own defaults to v7 dark or next dark , these issues will still be present.

@AMoo-Miki
Copy link
Contributor

AMoo-Miki commented Jul 31, 2023

To Kroosh's point, theme:version and theme:darkMode are configurable parameters in the opensearch_dashboards.yml file, and whether we decide to change the defaults or not, don't impact the need to make the pages function correctly for those configurations.

@stephen-crawford
Copy link
Contributor

stephen-crawford commented Aug 2, 2023

Hi @AMoo-Miki, I was looking at setting up the changes you mentioned.

I went over to the login-page.tsx and tried to add:

  const {
    services: { http, data, uiSettings, application },
  } = useOpenSearchDashboards<CoreStart & AppPluginStartDependencies>();

  const IS_DARK_THEME = uiSettings.get('theme:darkMode');

here: https://github.com/opensearch-project/security-dashboards-plugin/blob/91faa3ddd289bf60336e628eac4ae7c49cf39249/public/apps/login/login-page.tsx#L71C1-L71C1.

Based off of the pattern shown here: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/a52e95cbcc5075a4716274749d71d7d635704c4f/src/plugins/opensearch_dashboards_overview/public/components/overview/overview.tsx#L76-L81.

I was then going to add this: className={login-wrapper ${IS_DARK_THEME ? 'dark-theme' : 'light-theme'}} to the EUIListGroup thinking it would then change the color as required.

It seems like this is not the right method of handling this however...

Do you have any suggestions for what I can try? I also added the opensearchDashboardsReact dependency, so I am not sure what I am missing here for the uiSettings to be accessable in the Security Dashboards. Sorry if this is obvious, but I am less familiar with the front end code.

@stephen-crawford
Copy link
Contributor

stephen-crawford commented Aug 4, 2023

Hi @KrooshalUX, @AMoo-Miki, @kgcreative

For the login-page I have:

...

interface LoginPageDeps {
  http: CoreStart['http'];
  uiSettings: CoreStart['uiSettings'];
  config: ClientConfigType;
}

interface LoginButtonConfig {
  buttonname: string;
  showbrandimage: boolean;
  brandimage: string;
  buttonstyle: string;
}

function redirect(serverBasePath: string) {
  // navigate to nextUrl
  const urlParams = new URLSearchParams(window.location.search);
  let nextUrl = urlParams.get('nextUrl');
  if (!nextUrl || nextUrl.toLowerCase().includes('//')) {
    // Appending the next url with trailing slash. We do so because in case the serverBasePath is empty, we can simply
    // redirect to '/'.
    nextUrl = serverBasePath + '/';
  }
  window.location.href = nextUrl + window.location.hash;
}

export function LoginPage(props: LoginPageDeps) {
  const [username, setUsername] = React.useState('');
  const [password, setPassword] = React.useState('');
  const [loginFailed, setloginFailed] = useState(false);
  const [loginError, setloginError] = useState('');
  const [usernameValidationFailed, setUsernameValidationFailed] = useState(false);
  const [passwordValidationFailed, setPasswordValidationFailed] = useState(false);
  const IS_DARK_THEME = props.uiSettings.get('theme:darkMode');
  const defaultBrandImage = IS_DARK_THEME ? defaultDarkBrandImage : defaultLightBrandImage

...

This allows us to access the uiSettings from core start by modifying


export function renderApp(
  coreStart: CoreStart,
  params: AppMountParameters,
  config: ClientConfigType
) {
  ReactDOM.render(<LoginPage http={coreStart.http} uiSettings={coreStart.uiSettings} config={config} />, params.element);
  return () => ReactDOM.unmountComponentAtNode(params.element);
}

I am running into an issue with the use of this setting however. From what I have seen, when this configuration is used, we need elements to swap between:

From Overview.tsx

  const getSolutionGraphicURL = (solutionId: string) =>
    `/plugins/${PLUGIN_ID}/assets/solutions_${solutionId}_${
      IS_DARK_THEME ? 'dark' : 'light'
    }_2x.png`;

I am not sure where to get these elements from.

@stephen-crawford
Copy link
Contributor

Messaged Miki since it is not clear to me how to resolve the elements--I assume there are separate assets to be rendered but I do not know where to find these.

@stephen-crawford
Copy link
Contributor

Miki's change here: opensearch-project/oui#871 fixes the second problem

@AMoo-Miki
Copy link
Contributor

AMoo-Miki commented Aug 10, 2023

For the last item, I have refactored OSD's serving of logos in opensearch-project/OpenSearch-Dashboards#4702 and have the changes ready to PR to this repo as soon as that is merged.

@stephen-crawford
Copy link
Contributor

This is complete following: #1568

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants