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.

Sign up for GitHub

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

Jump to bottom

EPMRPP-89659 || Update notification tab layout #3795

Merged
merged 23 commits into from
Apr 19, 2024
Merged

EPMRPP-89659 || Update notification tab layout #3795

merged 23 commits into from
Apr 19, 2024

Conversation

maria-hambardzumian
Copy link
Contributor

@maria-hambardzumian maria-hambardzumian commented Apr 10, 2024 •
edited
Loading

PR Checklist

  • Have you verified that the PR is pointing to the correct target branch? (develop for features/bugfixes, other if mentioned in the task)
  • Have you verified that your branch is consistent with the target branch and has no conflicts? (if not, make a rebase under the target branch)
  • Have you checked that everything works within the branch according to the task description and tested it locally?
  • Have you run the linter (npm run lint) prior to submission? Enable the git hook on commit in your IDE to run it and format the code automatically.
  • Have you run the tests locally and added/updated them if needed?
  • Have you checked that app can be built (npm run build)?
  • Have you made sure that all the necessary pipelines has been successfully completed?
  • If the task requires translations to be updated, have you done this by running the manage:translations script?

Visuals

@maria-hambardzumian maria-hambardzumian changed the base branch from develop to feature/notifications April 11, 2024 10:35
Vadim73i
Vadim73i reviewed Apr 11, 2024
View reviewed changes
app/src/components/panels/helpPanel/index.js Outdated Show resolved Hide resolved
app/localization/translated/be.json Outdated Show resolved Hide resolved
.../inside/projectSettingsPageContainer/content/notifications/emptyRuleState/emptyRuleState.jsx Outdated Show resolved Hide resolved
.../inside/projectSettingsPageContainer/content/notifications/emptyRuleState/emptyRuleState.jsx Outdated Show resolved Hide resolved
.../inside/projectSettingsPageContainer/content/notifications/emptyRuleState/emptyRuleState.jsx Outdated Show resolved Hide resolved
...inside/projectSettingsPageContainer/content/notifications/emptyRuleState/emptyRuleState.scss Outdated Show resolved Hide resolved
...inside/projectSettingsPageContainer/content/notifications/emptyRuleState/emptyRuleState.scss Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/emptyRuleState/index.js Outdated Show resolved Hide resolved
BlazarQSO
BlazarQSO reviewed Apr 11, 2024
View reviewed changes
app/src/components/panels/helpPanel/helpPanel.jsx Outdated Show resolved Hide resolved
app/src/components/panels/helpPanel/helpPanel.jsx Outdated Show resolved Hide resolved
app/src/components/panels/helpPanel/helpPanel.jsx Outdated Show resolved Hide resolved
app/src/components/panels/helpPanel/helpPanel.scss Outdated Show resolved Hide resolved
.../inside/projectSettingsPageContainer/content/notifications/emptyRuleState/emptyRuleState.jsx Outdated Show resolved Hide resolved
...inside/projectSettingsPageContainer/content/notifications/emptyRuleState/emptyRuleState.scss Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/footer/footer.scss Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/footer/index.js Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/notifications.jsx Outdated Show resolved Hide resolved
AmsterGet
AmsterGet requested changes Apr 12, 2024
View reviewed changes
Copy link
Member

@AmsterGet AmsterGet left a comment

Choose a reason for hiding this comment

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

The common comment:
For all pages in the new design, use the colors specified in the newColors.scss file.
At this point in develop we have completely redesign all the 'settings pages', so all other pages are considered old.
In case of any doubts, please contact the team to ask questions or discuss.

app/src/components/panels/helpPanel/helpPanel.jsx Outdated Show resolved Hide resolved
app/src/components/panels/helpPanel/helpPanel.jsx Outdated Show resolved Hide resolved
.../inside/projectSettingsPageContainer/content/notifications/emptyRuleState/emptyRuleState.jsx Outdated Show resolved Hide resolved
...inside/projectSettingsPageContainer/content/notifications/emptyRuleState/emptyRuleState.scss Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/footer/footer.jsx Outdated Show resolved Hide resolved
...Container/content/notifications/modals/addEditNotificationModal/addEditNotificationModal.jsx Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/notifications.jsx Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/notifications.scss Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/notifications.scss Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/notifications.scss Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.65%. Comparing base (e697b28) to head (586cc5c).

Additional details and impacted files 
@@                  Coverage Diff                   @@
##           feature/notifications    #3795   +/-   ##
======================================================
  Coverage                  60.65%   60.65%           
======================================================
  Files                         79       79           
  Lines                        859      859           
  Branches                     123      123           
======================================================
  Hits                         521      521           
  Misses                       311      311           
  Partials                      27       27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AmsterGet
AmsterGet requested changes Apr 15, 2024
View reviewed changes
app/src/pages/inside/projectSettingsPageContainer/content/notifications/helpPanel/helpPanel.jsx Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.jsx Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.jsx Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.jsx Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.jsx Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.jsx Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.jsx Outdated Show resolved Hide resolved
...src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.scss Outdated Show resolved Hide resolved
...src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.scss Outdated Show resolved Hide resolved
...src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.scss Outdated Show resolved Hide resolved
@maria-hambardzumian
- fixes & additions
525ddb1
@maria-hambardzumian
Merge remote-tracking branch 'origin/feature/EPMRPP-89659-update-noti…
5b88cdc 
…fication-tab-layout' into feature/EPMRPP-89659-update-notification-tab-layout
Vadim73i
Vadim73i reviewed Apr 18, 2024
View reviewed changes
app/localization/translated/be.json Outdated Show resolved Hide resolved
app/localization/translated/be.json Outdated Show resolved Hide resolved
app/localization/translated/be.json Outdated Show resolved Hide resolved
app/localization/translated/ru.json Outdated Show resolved Hide resolved
app/localization/translated/uk.json Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/notifications.jsx Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/notifications.jsx Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.jsx Outdated Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.jsx Show resolved Hide resolved
app/src/pages/inside/projectSettingsPageContainer/content/notifications/ruleGroup/ruleGroup.jsx
</div>
</FieldElement>

{!isAvailable() ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove "not" and swap places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there big difference?

Copy link
Contributor

Choose a reason for hiding this comment

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

These are clean code best practices. Don't use 'not' when possible.

AmsterGet
AmsterGet requested changes Apr 19, 2024
View reviewed changes
Copy link
Member

@AmsterGet AmsterGet left a comment

Choose a reason for hiding this comment

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

When I'm trying to create the first rule, I've got the next error and warnings
image
image
image
The rule displayed only after page refresh.

@AmsterGet
Copy link
Member

Please also fix the Sonar Cloud issue

image

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.1% Duplication on New Code

See analysis details on SonarCloud

@maria-hambardzumian maria-hambardzumian merged commit 201168f into feature/notifications Apr 19, 2024
6 checks passed
@maria-hambardzumian maria-hambardzumian deleted the feature/EPMRPP-89659-update-notification-tab-layout branch April 19, 2024 13:26
AmsterGet pushed a commit that referenced this pull request Apr 19, 2024
@maria-hambardzumian @MariaHambardzumian @AmsterGet
EPMRPP-89659 || Update notification tab layout (#3795)
b8852d2 
* EPMRPP-89790 | updated notification modal (field for webhook url)

* EPMRPP-89790 | updated modal's field message

* EPMRPP-89790 | Implemented help panel section

* EPMRPP-89790 | Notification footer with appropriate data items

* EPMRPP-89790 | Notification footer style

* EPMRPP-89790 | Include WebhookURL in constants

* EPMRPP-89687 | Map enabled notification plugin group

* EPMRPP-89687 | Include empty state for each plugin

* EPMRPP-89687 | Update notification messages

* - changes from translations

* - added 2 icons for helpPanel

* EPMRPP-89687 | fixes to open modal

* - fix SonarLint

* - replaced attribute selector

* EPRRPP-89659 | Fixes

* EPRRPP-89659 | type prop validation

* - fixes & additions

* EPMRPP-89781 | Implement own toggler for each group

* EPMRPP-89781 | fixes

* EPMRPP-89781 | Sonar fix

* EPMRPP-89781 | remove lock action

---------

Co-authored-by: MariaHambardzumian <[email protected]>
maria-hambardzumian added a commit that referenced this pull request Apr 19, 2024
@maria-hambardzumian @MariaHambardzumian
EPMRPP-89659 || Update notification tab layout (#3795)
d5b8b19 
* EPMRPP-89790 | updated notification modal (field for webhook url)

* EPMRPP-89790 | updated modal's field message

* EPMRPP-89790 | Implemented help panel section

* EPMRPP-89790 | Notification footer with appropriate data items

* EPMRPP-89790 | Notification footer style

* EPMRPP-89790 | Include WebhookURL in constants

* EPMRPP-89687 | Map enabled notification plugin group

* EPMRPP-89687 | Include empty state for each plugin

* EPMRPP-89687 | Update notification messages

* - changes from translations

* - added 2 icons for helpPanel

* EPMRPP-89687 | fixes to open modal

* - fix SonarLint

* - replaced attribute selector

* EPRRPP-89659 | Fixes

* EPRRPP-89659 | type prop validation

* - fixes & additions

* EPMRPP-89781 | Implement own toggler for each group

* EPMRPP-89781 | fixes

* EPMRPP-89781 | Sonar fix

* EPMRPP-89781 | remove lock action

---------

Co-authored-by: MariaHambardzumian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BlazarQSO BlazarQSO BlazarQSO left review comments

@Vadim73i Vadim73i Vadim73i approved these changes

@AmsterGet AmsterGet AmsterGet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maria-hambardzumian @codecov-commenter @AmsterGet @BlazarQSO @Vadim73i @MariaHambardzumian