Loki: Add improvements to loki label browser

[gwdawson]

What is this feature?
this PR improves how the new loki label browser modal works.

• rather than disabling the button, we now display the error inside the modal
• improve the code quality of this component

Why do we need this feature?
following my first pr (#58525), i received some feedback with these improvements.

Who is this feature for?
...

Which issue(s) does this PR fix?:
...

Special notes for your reviewer:
...

[ivanahuckova]

Nice! Left some feedback related to implementation logic of loading labels within browser.

[ivanahuckova]

Suggested change

	{!hasLogLabels && <p>No labels found.</p>}
	{labelsLoaded && !hasLogLabels && <p>No labels found.</p>}

Should this be this?

[matyax]

That's why sometimes we prefer ternaries over all these booleans:

Suggested change

	{!hasLogLabels && <p>No labels found.</p>}
	return (
	<Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
	{!labelsLoaded ? (
	<LoadingPlaceholder text="Loading labels..." />
	) : (
	<>
	{!hasLogLabels ? (
	<p>No labels found.</p>
	) : (
	<LocalStorageValueProvider<string[]> storageKey={LAST_USED_LABELS_KEY} defaultValue={[]}>
	{(lastUsedLabels, onLastUsedLabelsSave, onLastUsedLabelsDelete) => {
	return (
	<LokiLabelBrowser
	languageProvider={datasource.languageProvider}
	onChange={onChange}
	lastUsedLabels={lastUsedLabels}
	storeLastUsedLabels={onLastUsedLabelsSave}
	deleteLastUsedLabels={onLastUsedLabelsDelete}
	app={app}
	/>
	);
	}}
	</LocalStorageValueProvider>
	)}
	</>
	)}
	</Modal>
	);

[gwdawson]

@matyax something like this was my initial thought but i'm not a huge fan on nested ternaries.

[matyax]

When you start seeing these piling up it's usually a better option:

[svennergr]

Imo just using early returns could offer best readability here, even though you may need to repeat <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}> lines.

e.g.:

if (!labelsLoaded) {
    return (
      <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
        <LoadingPlaceholder text="Loading labels..." />
      </Modal>
    );
  }
  if (!hasLogLabels) {
    return (
      <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
        <p>No labels found.</p>
      </Modal>
    );
  }
  return (
    <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
      <LocalStorageValueProvider<string[]> storageKey={LAST_USED_LABELS_KEY} defaultValue={[]}>
        {(lastUsedLabels, onLastUsedLabelsSave, onLastUsedLabelsDelete) => {
          return (
            <LokiLabelBrowser
              languageProvider={datasource.languageProvider}
              onChange={onChange}
              lastUsedLabels={lastUsedLabels}
              storeLastUsedLabels={onLastUsedLabelsSave}
              deleteLastUsedLabels={onLastUsedLabelsDelete}
              app={app}
            />
          );
        }}
      </LocalStorageValueProvider>
    </Modal>
  );

[matyax]

That's also a good option. What I don't like about that is 1) mounting and unmounting a Modal component during a transition (loading -> loaded with or without labels) and 2) having to 3x <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>.

[ivanahuckova]

Then what about keeping modal, just changing the modal content:

  const LogBrowserContent = () => {
    if (!labelsLoaded) {
      return <LoadingPlaceholder text="Loading labels..." />;
    }

    if (labelsLoaded && !hasLogLabels) {
      return <div>No labels found.</div>;
    }

    return (
      <LocalStorageValueProvider<string[]> storageKey={LAST_USED_LABELS_KEY} defaultValue={[]}>
        {(lastUsedLabels, onLastUsedLabelsSave, onLastUsedLabelsDelete) => {
          return (
            <LokiLabelBrowser
              languageProvider={datasource.languageProvider}
              onChange={onChange}
              lastUsedLabels={lastUsedLabels}
              storeLastUsedLabels={onLastUsedLabelsSave}
              deleteLastUsedLabels={onLastUsedLabelsDelete}
              app={app}
            />
          );
        }}
      </LocalStorageValueProvider>
    );
  };

  return (
    <Modal isOpen={isOpen} title="Label browser" onDismiss={onClose}>
      <LogBrowserContent />
    </Modal>
  );```
  

[svennergr]

This is still not in a place where the languageProvider was definitely started, or at least finished starting. Best would be to move this to the useEffect callback.

[svennergr]

LGTM. I am unsure if @matyax has strong opinions about the structure of the components though.

[matyax]

I think this is not ideal, but it's still valid react, so 👍