Closes #892 - Sidepanel for showing version history of the workspace added #900

fylip97 · 2020-08-06T12:15:07Z

This change is

mariusoe

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @fylip97)

a discussion (no related file):
Imo it's maybe better to move the sidepanel besides the editor itself, so we don't effect the toolbar. See the following example:

So we have a small side-toolbar with some buttons.

When a button is clicked, the related panel is show.

This has the advantage that we could simply add more panels to it, if needed.

a discussion (no related file):
Can you try making it more like a list?

a discussion (no related file):
Now, when seeing it like this that the "version" are being disabled for highlight whether they contain a changed file, it is not the best way doing this in my oppinion. It is very confusing why the entries are suddenly being disabled and I think users will not understand this.

Maybe it is better keeping them as they are but just adding an icon or some small indicator which shows that certain versions containg changes of the selected file.

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

    isMoveDialogShown: false,
    filePath: null,
    fileTreeSelected: '',

Remove this state variable. The selection is already persisted in the Redux state. Your component can fetch it from there.

See the file components\views\promotion\PromotionView.js on how you can access the redux state using hooks (e.g.: const liveCommitId = useSelector((state) => state.promotion.liveCommitId);)

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

            showMoveDialog={this.showMoveDialog}
            readOnly={readOnly}
            selectedFile={this.selctedFile}

See previous comment. Not needed.

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

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

Not needed. See comments in the ConfigurationView.js

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 6 at r1 (raw file):

import { Button } from 'primereact/button';

const contentItem = (item, fileTreeSelected) => {

Please move this component to a separate file.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 7 at r1 (raw file):

const contentItem = (item, fileTreeSelected) => {
  let commitDisable = true;

Maybe name this differently, like containsChanges. Then we can use this to "mark" or highlight the version. Disabling would be little bit confusing in case we use a different style for highlighting them.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 10 at r1 (raw file):

  if (fileTreeSelected != '') {
    for (let i = 0; i < item.changes.length; i++) {
      const itemChanges = item.changes[i].slice(0, fileTreeSelected.length);

This is not working.

For example, in case you have an element selected name /a you always compare only the first two characters.
Thus, if you have a changed file, e.g. /anyFile.yml and comparing the first two characters (because the length of the selected file name is two) would result in a positive match.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 60 at r1 (raw file):

            display: table;
            margin: auto;
            font-weight: bold;

Please use only few bold text. For example, only for the name.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 66 at r1 (raw file):

      <div className={!commitDisable ? 'this' : 'this-disable'}>
        <div>
          <table className="table">

Try to use flex boxes instead of a table. They are bit more complicated at the beginning, but it's worth the effort ;)

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 71 at r1 (raw file):

                <td className="td">
                  <label className="name">{item.name}</label>
                  <label className="id">({item.id})</label>

Try writing the text in a grey tone and leave away the brackets.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 90 at r1 (raw file):

};

class HistoryView extends React.Component {

Please refactor this class into a functional component and use React Hooks for the state management.

Example: \components\views\promotion\PromotionFileView.js

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 94 at r1 (raw file):

    super(props);
    this.state = {
      visibleLeft: true,

Can be removed. Not used.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 95 at r1 (raw file):

    this.state = {
      visibleLeft: true,
      selectedCommit: this.props.data[0],

Can be removed. Not used.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 130 at r1 (raw file):

            }
          `}
        </style>

please add a empty line between the style and the div tag.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 138 at r1 (raw file):

            <div className="content">
              {this.props.data.map((item, index) => {
                return (

return is not needed.
You can write this also like this:

{this.props.data.map((item, index) => (
  <div className="template" key={index}>
    ...
  </div>
))}

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/history/HistoryView.js, line 141 at r1 (raw file):

this.props.fileTreeSelected

Please don't use this.props... and this.state... in the JSX code but destcrut them at the beginning of the render method.

const {xxx, yyy, zzz} = this.props;
const {abc} = this.state;

By doing this, you don't have to write this.props all the time in the JSX code and you immediatly see which props and state variable are necessary by the code.

components/inspectit-ocelot-configurationserver-ui/src/components/views/promotion/PromotionSidebar.js, line 10 at r1 (raw file):

 * @param {object} option current list option
 */
const selectionTemplate = ({ file, type, authors, isApproved, canSelfApprove, currentUser, canPromote }) => {

Why are these changes here? Please reset this file and remove it from the PR.

fylip97

Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @fylip97 and @mariusoe)

a discussion (no related file):