Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closes #803 and #804 - Users may now define custom promotion messages. #864

Merged

Conversation

MariusBrill
Copy link
Member

@MariusBrill MariusBrill commented Jul 24, 2020

This change is Reviewable

Copy link
Member

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mbrill-nt)


components/inspectit-ocelot-configurationserver/src/test/java/rocks/inspectit/ocelot/rest/configuration/PromotionControllerIntTest.java, line 68 at r1 (raw file):

        @Test
        public void promoteNoFiles() {

Can you add a test-case with a missing promotion message?


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

    const footer = (
      <div>
        <Button label="Promote" onClick={this.promote} disabled={this.state.promotionDisabled} />

No need to have promotionDisabled in your state. You can just use
disabled={isLoading || !promotionMessage}
here.


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

            ref={this.input}
            style={{ width: '100%' }}
            placeholder="Enter description..."

Change to Describe the changes...


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

            value={promotionMessage}
            onChange={(e) => this.setState({ promotionMessage: e.target.value })}
            onKeyUp={() => this.setState({ promotionDisabled: promotionMessage >= 0 && !this.props.isLoading })}

Remove this callback, not needed if you remove promotionDisabled from the state.


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

  componentDidUpdate() {
    if (this.state.resetSelection) {

Don't do this via a state variable.

Have you tried using componentDidMount instead?


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

  onKeyPress = (e) => {
    if (e.key === 'Enter' && !this.state.promotionDisabled) {

See the first comment, you can replace this.state.promotionDisabled with !isLoading && promotionMessage


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

  };

  onShow = () => {

You should use the onShow prop of the Dialog component in your render method instead.

Copy link
Member

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@JonasKunz JonasKunz merged commit f55607d into inspectIT:master Jul 28, 2020
@MariusBrill MariusBrill deleted the User-defined_promotion_messages branch May 25, 2021 10:47
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
2 participants