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

Alerting Tab and minor bug fixes #1601

Merged
merged 1 commit into from
May 2, 2023
Merged

Conversation

Dimi-Ma
Copy link
Member

@Dimi-Ma Dimi-Ma commented May 2, 2023

#1547
#1600


This change is Reviewable

Copy link
Contributor

@quandor quandor left a comment

Choose a reason for hiding this comment

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

Looks good to me except those 2 things I mentioned.
In general I would prefer to make two distinct commits. One for each issue that you fixed. In that way it is easier for the reviewer and it is easier for someone who wants to know what caused an issue.

You don't have to rework it this time. But in the future this might be good idea.

There are case where this is not easily done. But that might be an indication that those issues are related to each other or even the same.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Dimi-Ma)


components/inspectit-ocelot-configurationserver-ui/src/components/views/alerting/AlertingView.js line 116 at r1 (raw file):

      `}</style>
      <div className="this">
        <TabMenu className="menu" model={items} activeitem={activeTab} onTabChange={(e) => setActiveTab(e.value)} />

Was this change intentional? Since all other attributes are in camel case, I wonder why activeitem should not be.


components/inspectit-ocelot-configurationserver-ui/src/components/views/alerting/rules/tree/AlertingRulesTree.js line 4 at r1 (raw file):

import { useDispatch, useSelector } from 'react-redux';
import classNames from 'classnames';
import classnames from 'classnames';

Do we need that 2nd import?

Copy link
Member Author

@Dimi-Ma Dimi-Ma left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @quandor)


components/inspectit-ocelot-configurationserver-ui/src/components/views/alerting/AlertingView.js line 116 at r1 (raw file):

Previously, quandor (Jochen Just) wrote…

Was this change intentional? Since all other attributes are in camel case, I wonder why activeitem should not be.

Yes this is intentional because it leads to an error message. Since i was not sure if the activeTab was needed for underlying functionality, i kept it as a custom attribute for the DOM.


components/inspectit-ocelot-configurationserver-ui/src/components/views/alerting/rules/tree/AlertingRulesTree.js line 4 at r1 (raw file):

Previously, quandor (Jochen Just) wrote…

Do we need that 2nd import?

Yes we need both, because "classnames" is a function call and "classNames" a namespace declaration.

Copy link
Contributor

@quandor quandor left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Dimi-Ma)


components/inspectit-ocelot-configurationserver-ui/src/components/views/alerting/rules/tree/AlertingRulesTree.js line 4 at r1 (raw file):

Previously, Dimi-Ma (Dimitiros Mantas) wrote…

Yes we need both, because "classnames" is a function call and "classNames" a namespace declaration.

Oh. I did not recognize the different casing.

@Dimi-Ma Dimi-Ma merged commit ba1005f into inspectIT:master May 2, 2023
@quandor quandor added the bug Something isn't working label May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants