EPMRPP-78035 || Launch and Test item description validation update #3188

tr1ble · 2022-07-22T08:06:31Z

No description provided.

codecov-commenter · 2022-07-22T08:13:31Z

Codecov Report

Merging #3188 (8a60d54) into develop (4c2811d) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

@@             Coverage Diff             @@
##           develop    #3188      +/-   ##
===========================================
- Coverage    59.70%   59.67%   -0.03%     
===========================================
  Files           75       75              
  Lines          804      806       +2     
  Branches       118      118              
===========================================
+ Hits           480      481       +1     
- Misses         296      297       +1     
  Partials        28       28

Impacted Files	Coverage Δ
...pp/src/common/utils/validation/commonValidators.js	`0.00% <0.00%> (ø)`
app/src/common/utils/validation/validate.js	`96.77% <100.00%> (+0.05%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

chivekrodis

Solution doesn't work
error is not appear after entering more than 2048 symbols
info hint not appears after entering 1000 symbols(comment to the task)

chivekrodis · 2022-07-22T08:27:51Z

app/src/components/fields/fieldErrorHint/fieldErrorHint.jsx

+  descriptionHint: {
+    id: 'EditItemModal.descriptionHint',
+    defaultMessage: "Description should have size from '0' to '2048' symbols",
+  },


According to the task there is should be a hint for each level like(launch, suite, test, step)

As we use the same modal for launch, suite and so on, I've asked to use one message for all levels

As I know we have selector to detect level
Also in case of any changes of the task you should ask to update it to avoid misunderstanding between owner and reviewers, qa team

chivekrodis · 2022-07-22T08:30:52Z

app/src/components/main/markdown/markdownEditor/markdownEditor.scss

@@ -269,3 +272,23 @@
    }
  }
 }
+
+@mixin additional-text($color) {
+  position: relative;


What the reason using position here? Why not margin?

Because it doesn't work in another case

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/span
https://www.w3.org/TR/CSS21/box.html#margin-properties

chivekrodis · 2022-07-22T08:32:02Z

app/src/components/main/markdown/markdownEditor/markdownEditor.scss

@@ -23,6 +23,9 @@
  &.focused {
    border-color: $COLOR--topaz;
  }
+  &.invalid {
+    border-color: $COLOR--orange-red;


Doesn't match design

chivekrodis · 2022-07-22T08:36:31Z

app/src/components/main/markdown/markdownEditor/markdownEditor.jsx

+    active: PropTypes.bool,
+    touched: PropTypes.bool,
+    error: PropTypes.string,
+    hint: PropTypes.string,


Why did you add hint here? Yoг don't provide this props anywhere. But task have comment about it check it

chivekrodis · 2022-07-22T08:38:19Z

app/src/components/main/markdown/markdownEditor/markdownEditor.scss

+}
+
+.error {
+  @include additional-text($COLOR--red-failed-2);


Doesn't match design
but maybe border color and error color should have the same color

Yes, I think so too

chivekrodis · 2022-07-22T08:56:17Z

app/src/components/main/modal/modalLayout/modalFooter/modalFooter.scss

+    &.color-orange {
+      color: $COLOR--black-1;
+    }


This is confusing. color orange but actually black

chivekrodis · 2022-07-22T08:59:34Z

app/src/components/main/modal/modalLayout/modalLayout.jsx

@@ -182,6 +184,7 @@ export class ModalLayout extends Component {
    const {
      title,
      warningMessage,
+      warningColor,


Maybe better provide warning type or appearance and rely on it f.e we have warningType = info, so he is responsible for making the icon orange and the text black

chivekrodis · 2022-07-22T09:01:58Z

app/src/components/main/modal/modalLayout/modalFooter/modalFooter.scss

+    &.color-orange {
+      color: $COLOR--black-1;
+    }


This confuses. color orange but actually black
Maybe better provide warning type or appearance and rely on it f.e we have warningType = warn, so he is responsible for making the icon orange and the text black
error make them both red
and so on

Yeah, I agree, I've though about it too, but in this case we have to update all used footers

I think its Ok to update all warning appearance as they are rendered by same modal layout.

AmsterGet · 2022-07-22T08:47:26Z

app/localization/translated/be.json

@@ -515,6 +515,7 @@
  "EditItemModal.attributesLabel": "Атрыбуты",
  "EditItemModal.contentTitle": "Апісанне {type}а",
  "EditItemModal.descriptionPlaceholder": "Увядзіце апісанне {type}а",
+  "EditItemModal.descriptionHint": "Апісанне павінна мець памер ад '0' да '2048' сімвалаў",


Please use the same quotes everywhere.

AmsterGet · 2022-07-22T09:26:47Z

app/src/components/main/modal/modalLayout/modalFooter/modalFooter.scss

+    &.color-orange {
+      color: $COLOR--black-1;
+    }


I think its Ok to update all warning appearance as they are rendered by same modal layout.

chivekrodis

Are you sure that error is appear?

chivekrodis · 2022-07-22T10:15:59Z

app/src/components/main/markdown/markdownEditor/markdownEditor.jsx

+          this.simpleMDE &&
+          condition(this.simpleMDE.value()) && <div className={cx('hint')}>{text}</div>


As I see MarkdownEditor has value props maybe we can use it instead of
this.simpleMDE && condition(this.simpleMDE.value())
Also as MarkdownEditor is kind of common component if I want provide hint somewhere else I should always provide condition function even if I don't needed it(bcs without it it's leads to error) so maybe default hint should looks like

hint: { hintText: '', hintConditionFn: () => true }

Yeah, I saw the error.
value doesn't work. I've tried it first.

chivekrodis · 2022-07-22T10:47:13Z

app/src/components/main/modal/modalLayout/modalFooter/modalFooter.jsx

@@ -138,10 +138,10 @@ export class ModalFooter extends Component {
        )}
        {warningMessage && (
          <div className={cx('warning-block')}>
-            <i className={cx('warning-icon', { [`color-${warningColor}`]: warningColor })}>
+            <i className={cx('warning-icon', { [warningType]: warningType })}>


Maybe type-${warningType} and &.warning-type-info in scss to have more vision in the scss

chivekrodis · 2022-07-22T10:47:41Z

app/src/components/main/modal/modalLayout/modalLayout.jsx

@@ -85,7 +85,7 @@ export class ModalLayout extends Component {
    title: '',
    children: null,
    warningMessage: '',
-    warningColor: '',
+    warningType: '',


We don't have any default state?

chivekrodis · 2022-07-22T11:20:34Z

app/src/pages/inside/common/modals/editItemModal/editItemModal.jsx

@@ -105,6 +105,10 @@ const messages = defineMessages({
    id: 'TestItemDetailsModal.parametersLabel',
    defaultMessage: 'Parameters:',
  },
+  descriptionHint: {
+    id: 'EditItemModal.descriptionAdviceHint',
+    defaultMessage: 'You used 1000 of 2048 symbols',


This is static text, so the user can enter 2048 characters but still see that text, maybe the idea was to dynamically count?

I'll ask UX.

UX said that thought about static text, but if possible we can improve it to dynamic

…3188)

EPMRPP-78035 || Launch and Test item description validation update

c3e4021

tr1ble requested review from AmsterGet, chivekrodis and Vadim73i July 22, 2022 08:06

chivekrodis requested changes Jul 22, 2022

View reviewed changes

AmsterGet reviewed Jul 22, 2022

View reviewed changes

EPMRPP-78035 || Code review fixes - 1

65a67df

tr1ble requested review from chivekrodis and AmsterGet July 22, 2022 10:03

tr1ble added 2 commits July 22, 2022 14:05

EPMRPP-78035 || Code review fixes - 2

a817f87

EPMRPP-78035 || Code review fixes - 3

9e0eca1

chivekrodis requested changes Jul 22, 2022

View reviewed changes

tr1ble added 2 commits July 22, 2022 15:14

EPMRPP-78035 || Code review fixes - 4

af30bb7

EPMRPP-78035 || Code review fixes - 5

cab2d8d

tr1ble requested a review from chivekrodis July 22, 2022 12:43

EPMRPP-78035 || Code review fixes - 6

a56e1c8

chivekrodis approved these changes Jul 25, 2022

View reviewed changes

EPMRPP-78035 || Demo fixes

8a60d54

AmsterGet approved these changes Jul 25, 2022

View reviewed changes

AmsterGet merged commit 8094249 into develop Jul 25, 2022

AmsterGet deleted the EPMRPP-78035_Description_validation branch July 25, 2022 10:58

Vadim73i pushed a commit that referenced this pull request Sep 12, 2022

EPMRPP-78035 || Launch and Test item description validation update (#…

ca60a2b

…3188)

Vadim73i pushed a commit that referenced this pull request Oct 7, 2022

EPMRPP-78035 || Launch and Test item description validation update (#…

8b8d87b

…3188)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EPMRPP-78035 || Launch and Test item description validation update #3188

EPMRPP-78035 || Launch and Test item description validation update #3188

tr1ble commented Jul 22, 2022

codecov-commenter commented Jul 22, 2022 •

edited

Loading

chivekrodis left a comment

chivekrodis Jul 22, 2022

tr1ble Jul 22, 2022

chivekrodis Jul 22, 2022

chivekrodis Jul 22, 2022

tr1ble Jul 22, 2022

chivekrodis Jul 22, 2022

chivekrodis Jul 22, 2022

chivekrodis Jul 22, 2022

chivekrodis Jul 22, 2022

tr1ble Jul 22, 2022

chivekrodis Jul 22, 2022

chivekrodis Jul 22, 2022

chivekrodis Jul 22, 2022

tr1ble Jul 22, 2022 •

edited

Loading

AmsterGet Jul 22, 2022

AmsterGet Jul 22, 2022

AmsterGet Jul 22, 2022

chivekrodis left a comment

chivekrodis Jul 22, 2022

tr1ble Jul 22, 2022

chivekrodis Jul 22, 2022

chivekrodis Jul 22, 2022

chivekrodis Jul 22, 2022

tr1ble Jul 22, 2022

tr1ble Jul 22, 2022

		this.simpleMDE &&
		condition(this.simpleMDE.value()) && <div className={cx('hint')}>{text}</div>

EPMRPP-78035 || Launch and Test item description validation update #3188

EPMRPP-78035 || Launch and Test item description validation update #3188

Conversation

tr1ble commented Jul 22, 2022

codecov-commenter commented Jul 22, 2022 • edited Loading

Codecov Report

chivekrodis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tr1ble Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chivekrodis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 22, 2022 •

edited

Loading

tr1ble Jul 22, 2022 •

edited

Loading