intelowlproject / IntelOwl

IntelOwl: manage your Threat Intelligence at scale
https://intelowlproject.github.io
GNU Affero General Public License v3.0
3.73k stars 425 forks source link

Same component for the cells in plugin and job tabs #2051

Closed drosetti closed 5 months ago

drosetti commented 8 months ago

There are several fields that shows a string (ex: name, user, playbook_execute, etc...) and all of them have the same UX (allow to copy text with a single click and truncate with ... in case the text is too long for the column size). Have a component for all of them will improve the code and the maintenance.

aryan-bhokare commented 7 months ago

Hi @drosetti I have a few questions. Do we want to make a component for the copytoclipboard and tuncate part?

drosetti commented 6 months ago

Yes, I think a component to place where is required this kind of UX in the tables (job and plugin) it will reduce errors: in this way we will avoid copy-paste of code for each column

aryan-bhokare commented 6 months ago

So basically we want a component for the column of the table. this part should be made into component with properties like header, id, accessor, cell ... and others. Am I on the right path?

image

drosetti commented 6 months ago

No , I meant to create a single component for the Cell element. In fact this issue is a bit outdated: some of the fields mentioned in the issue (name, user) are working correctly.

I think there are a UX problem with these fields:

aryan-bhokare commented 6 months ago

Okay now I understand. Can you assign this issue to me, I can start working on it. Thanks!

aryan-bhokare commented 6 months ago

I think making a component out of it is kinda complicated

there are only few common blocks which repeats the same way.

      <CopyToClipboardButton
        showOnHover
        id={`table-md5-${job?.id}`}
        key={`table-md5-${job?.id}`}
        text={value}
        className="d-block text-truncate"
      >
        {value}
      </CopyToClipboardButton>
        <div className="d-flex justify-content-center">
          <span className="d-block text-break">
            {value}
          </span>
        </div>

we can only make components of these two or three logics. Other than that most of them are very different from each other. As you can see most of the jsx elements in the cell have very different code.

      <div className="d-flex justify-content-center mx-2">
        <PluginHealthCheckButton
          pluginName={value.name}
          pluginType_={PluginsTypes.INGESTOR}
        />
        <PluginPullButton
          pluginName={value.name}
          pluginType_={PluginsTypes.INGESTOR}
        />
      </div>
      <ul
        key={`visualizers-playbooks__${value}`}
        className="d-flex flex-column align-items-start"
      >
        {value?.sort().map((val) => (
          <li key={val}>
            <CopyToClipboardButton
              showOnHover
              id={`table-user-${val}`}
              key={`table-user-${val}`}
              text={val}
              className="d-block text-truncate"
            >
              {val}
            </CopyToClipboardButton>
          </li>
        ))}
      </ul>

We could do that to but it will have a nested conditional rendering which is not very readable.

One more solutions is to create a entirely new component which can dynamically handle different cells.

I would like to know your opinion and how we should move forward.

drosetti commented 6 months ago

We could do that to but it will have a nested conditional rendering which is not very readable.

I agree with you it will be a very complicated, component hard to read and maintain.

I think we can create a component with the CopyToClipboardButton inside other html tags (I'm thinking about div and span currently used, but if required other tags) with the bootstrap properties to make it responsive and use it with different params. In the Cell we will use it, for example:

<ul
        key={`visualizers-playbooks__${value}`}
        className="d-flex flex-column align-items-start"
      >
        {value?.sort().map((val) => (
          <li key={val}>
            <NewComponent value={val} />
          </li>
        ))}
      </ul>

What do you think about this solution ?

aryan-bhokare commented 6 months ago

Yes I think your approach is the better one. I was thinking something similar.

The new component might look something like this...

image

I am thinking about a component something like this which will be rendering 3 types of elements on the basis of props.

Like if we want copy to clipboard button it will render it accordingly. And is not CopytoClipboard we can use a text-wrap.

Text wrap would look like this. image

drosetti commented 6 months ago

I don't get why we cannot use always both CopyToClipboardButton and the text-wrap component, can you explain me ?

aryan-bhokare commented 6 months ago

Sorry I did not write correctly we can either have wrap or truncate . Right now we have truncate used in copytoclipboard.

aryan-bhokare commented 6 months ago

@drosetti How would you like me to proceed? Is the way we just discussed looks right to you?

drosetti commented 6 months ago

Yes, I think this is a good way, you can proceed. In case you have any doubt, let me know ;)

mlodic commented 5 months ago

solved in v6.0.0