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

Closes #1403 : [Feature] - Add possibility to use Multiple Editor Tabs in the Config Server UI #1498

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

salim-1997
Copy link
Member

@salim-1997 salim-1997 commented Jul 14, 2022

Closes #1403

  • new dependency "react-dyn-tabs" was added for implementing closable tabs.

This change is Reviewable

@aaronweissler aaronweissler self-assigned this Jul 22, 2022
Copy link
Member

@aaronweissler aaronweissler left a comment

Choose a reason for hiding this comment

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

A general thing is, the filename in the editor-toolbar is not updated when switching tabs by clicking on the tab itself, but only when clicking on files in the file-tree. I think that functionality should still be added.

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @salim-1997)


components/inspectit-ocelot-configurationserver-ui/src/components/editor/EditorView.js line 17 at r1 (raw file):

import 'react-dyn-tabs/themes/react-dyn-tabs-card.css';
import useDynTabs from 'react-dyn-tabs';
import { set } from 'lodash';

set can be removed since it is unused


components/inspectit-ocelot-configurationserver-ui/src/components/editor/EditorView.js line 52 at r1 (raw file):

}) => {
  const [options, setOptions] = useState({
    tabs: new Array(),

I would use [] instead of new Array()


components/inspectit-ocelot-configurationserver-ui/src/components/editor/EditorView.js line 56 at r1 (raw file):

  });
  useEffect(() => {
    if (name && value) {

value should be explicitly checked for being null because as it is now, empty files where value is '' will not be opened at all.


components/inspectit-ocelot-configurationserver-ui/src/components/editor/EditorView.js line 60 at r1 (raw file):

        _instance = instance;
      });
      let newInstance = {

If I understand the meaning of the variable instance correctly, I would rename this variable to something like newTab, since instance is something different than a tab, right?


components/inspectit-ocelot-configurationserver-ui/src/components/editor/EditorView.js line 61 at r1 (raw file):

      });
      let newInstance = {
        id: name,

name can not be used as the id, since it means that you can not open two files with the same filename at once.
We could usepath that the ConfigurationView creates in the same line as name as the id, since it will be unique per file. And then use name only for the tab's title.
I would also rename them in the EditorView's props to something like currentFilePath and currentFileName


components/inspectit-ocelot-configurationserver-ui/src/components/editor/EditorView.js line 81 at r1 (raw file):

      _instance.select(name);
      setOptions(() => {
        let isExisting = options.tabs.some((e) => {

Could this check and the related if-statement maybe be moved upwards in the useEffect method, so we skip creating the newInstance entirely if it is not needed anyways, because a tab already exists for the opened file?


components/inspectit-ocelot-configurationserver-ui/src/components/editor/EditorView.js line 84 at r1 (raw file):

          return e.id === newInstance.id;
        });
        console.log('isExisting', isExisting);

The console-log should be removed


components/inspectit-ocelot-configurationserver-ui/src/components/editor/EditorView.js line 91 at r1 (raw file):

      });
    }
  }, [name]);

value needs to be in here too, because it is used in the if statement above in line 56


components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/ConfigurationView.js line 2 at r1 (raw file):

import yaml from 'js-yaml';
import React, { useMemo } from 'react';

useMemo can be removed


components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/ConfigurationView.js line 70 at r1 (raw file):

    isConfigurationDialogShown: false,
    isConvertDialogShown: false,
    tabs: new Array(),

tabs can be removed since it is unused here

Code quote:

ConfigurationV

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/ConfigurationView.js line 229 at r1 (raw file):

            showMoveDialog={this.showMoveDialog}
            readOnly={readOnly}
            addToChildren={() => {}}

addToChildren can be removed, since it is not doing anything


components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/ConfigurationView.js line 254 at r1 (raw file):

          onToggleVisualConfigurationView={toggleVisualConfigurationView}
          sidebar={<ConfigurationSidebar />}
          // tabs={this.state.tabs}

comment can be removed


components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/FileTree.js line 41 at r1 (raw file):

      if (newSelection !== selection && newSelection !== selectedDefaultConfigFile) {
        this.props.selectFile(newSelection);
        this.props.addToChildren();

addToChildren can be removed, since it is not doing anything

Code quote:

addToChildre

components/inspectit-ocelot-configurationserver-ui/src/redux/ducks/configuration/actions.js line 388 at r1 (raw file):

export const addTab = () => ({
  type: types.INCREMENT_TABS,
});

If I understand correctly, this and the related code in reducers.js and types.js is not needed anymore, or is it?

Code quote:

export const addTab = () => ({
  type: types.INCREMENT_TABS,
});

@aaronweissler aaronweissler removed their assignment Aug 15, 2022
@aaronweissler
Copy link
Member

Converted this to draft for now, since we decided to wait for the upgraded PrimeReact from #1460 to be able to use its tabs-functionality instead of the library used here right now

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.

[Feature] - Add possibility to use Multiple Editor Tabs in the Config Server UI
3 participants