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

Fix #3046 UI Add support for request description #3074

Merged
merged 40 commits into from Mar 4, 2022
Merged

Fix #3046 UI Add support for request description #3074

merged 40 commits into from Mar 4, 2022

Conversation

Sachin-chaurasiya
Copy link
Member

@Sachin-chaurasiya Sachin-chaurasiya commented Mar 2, 2022 •
edited

Closes #3046

Describe your changes :

I worked on issue #3046.

Type of change :

  • New feature

Frontend Preview (Screenshots) :

Checklist:

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my own.
  • I have tagged my reviewers below.
  • I have commented on my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • All new and existing tests passed.

Reviewers

@shahsank3t, @darth-coder00

@Sachin-chaurasiya Sachin-chaurasiya marked this pull request as draft March 2, 2022 11:01
@Sachin-chaurasiya Sachin-chaurasiya self-assigned this Mar 2, 2022
vivekratnavel
vivekratnavel suggested changes Mar 2, 2022
View reviewed changes
Copy link
Contributor

@vivekratnavel vivekratnavel left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. I have added inline comments/suggestions to improve.

openmetadata-ui/src/main/resources/ui/src/pages/service/index.tsx Outdated Show resolved Hide resolved
openmetadata-ui/src/main/resources/ui/src/pages/service/index.tsx Outdated Show resolved Hide resolved
openmetadata-ui/src/main/resources/ui/src/utils/FeedElementUtils.tsx Outdated Show resolved Hide resolved
@vivekratnavel
Copy link
Contributor

vivekratnavel commented Mar 2, 2022 •
edited by Sachin-chaurasiya

I understand that this patch is still a work in progress. But, just want to mention the following list of things that needs to be taken care of. Some are functional and others are cosmetic that is not urgent:

  • Request description should be a small button (cosmetic)

Screen Shot 2022-03-02 at 11 33 28 AM

  • The comment icon should be aligned at some fixed position for all the rows (cosmetic)

Screen Shot 2022-03-02 at 11 33 40 AM

  • We should show a comment icon with (+) to start a new conversation when there is already a description in place. This should also apply to tags so that users can suggest, request or comment on the tags column (functional)

Screen Shot 2022-03-02 at 11 34 57 AM

  • Here, the total number of replies is 2. So, the text should read "1 older reply" and show only the user icon of that one user (functional)

Screen Shot 2022-03-02 at 11 41 34 AM

  • The request description button should not be shown when the user has permission to edit the description. Instead, the comment icon with (+) should be shown to start a conversation (functional) (NA)

Good to have to improve UX:

  • When a user edits the description or a tag, a new conversation thread is created, but the UI will not show it unless the page is refreshed. It will be good to fetch thread count and update the Activity Feed tab count and the description row with the newly created conversation for the user to open the conversation immediately. This happens when the user requests a description but not while editing.

@Sachin-chaurasiya
Copy link
Member Author

@vivekratnavel, Thank you for the review. I'll look into it.

@Sachin-chaurasiya Sachin-chaurasiya marked this pull request as ready for review March 3, 2022 14:19
@Sachin-chaurasiya Sachin-chaurasiya linked an issue Mar 3, 2022 that may be closed by this pull request
Activity Feed: showing replies when there are no more replies #3106
Closed
@vivekratnavel
Copy link
Contributor

  • This placeholder looks good. One minor improvement could be to include the actual icon in the place of "chat icon" text.

Screen Shot 2022-03-03 at 10 36 21 AM

Something like this

Screen Shot 2022-03-03 at 10 42 03 AM

  • The "Start new conversation" icon renders with different sizes and the position of (+) is not consistent.

Screen Shot 2022-03-03 at 10 34 45 AM

  • Also, the start conversation icon should only be shown on hover just like the edit icon. The tags column should also show this icon on hover to start a new conversation around tags.
  • The request description text here should be a button to improve UX. Here it matches with the same color as the disabled "No description added" text and doesn't look like an action item that the user can click on. Also, in my opinion, the text can be "No description"

Screen Shot 2022-03-03 at 10 46 53 AM

It can even be as subtle as this not to grab attention but be present everywhere. The button can brighten up on hover with a different border color (primary)

Screen Shot 2022-03-03 at 11 00 25 AM

@Sachin-chaurasiya Sachin-chaurasiya mentioned this pull request Mar 4, 2022
Closed
darth-coder00
darth-coder00 suggested changes Mar 4, 2022
View reviewed changes
Copy link
Contributor

@darth-coder00 darth-coder00 left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM!

openmetadata-ui/src/main/resources/ui/src/utils/SvgUtils.tsx Outdated Show resolved Hide resolved
darth-coder00
darth-coder00 approved these changes Mar 4, 2022
View reviewed changes
Copy link
Contributor

@darth-coder00 darth-coder00 left a comment

Choose a reason for hiding this comment

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

LGTM!

@darth-coder00 darth-coder00 self-requested a review March 4, 2022 15:36
@sonarcloud
Copy link

sonarcloud bot commented Mar 4, 2022

[catalog] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@vivekratnavel
Copy link
Contributor

Selenium tests are failing and some of them might be related to the changes in this patch:

2022-03-04T19:11:30.4443127Z [ERROR] Failures: 
2022-03-04T19:11:30.4454436Z [ERROR]   CommonTests.addMultipleTagsCheck:196 expected [11] but found [0]
2022-03-04T19:11:30.4454848Z [ERROR]   CommonTests.searchNotShowingResultsCheck:409 No search results found
2022-03-04T19:11:30.4456594Z [ERROR]   DashboardDetailsPageTest.editChartDescription:180 Description not updated
2022-03-04T19:11:30.4458326Z [ERROR]   DashboardServiceTestPage.checkConnectionConfigTab:137 Error while updating service
2022-03-04T19:11:30.4460066Z [ERROR]   DatabaseServicePageTest.checkConnectionConfigTab:159 Error while updating service
2022-03-04T19:11:30.4461845Z [ERROR]   MyDataPageTest.checkRecentlyViewed:274 expected [dim_address] but found [shopify]
2022-03-04T19:11:30.4462772Z [ERROR]   PipelineDetailsPageTest.editTaskDescription:187 Description not updated
2022-03-04T19:11:30.4463292Z [ERROR]   RolesPageTest.addRole:90 Role not added
2022-03-04T19:11:30.4464027Z [ERROR]   RolesPageTest.addRuleWithoutOperation:222->addRole:90 Role not added
2022-03-04T19:11:30.4464530Z [ERROR]   TagsPageTest.addTagWithExistingName:231 expected [Not Used] but found [1]
2022-03-04T19:11:30.4465066Z [ERROR]   TeamsPageTest.addAsset:166->createTeam:102 Team not added
2022-03-04T19:11:30.4465546Z [ERROR]   TeamsPageTest.addUser:112->createTeam:102 Team not added
2022-03-04T19:11:30.4466085Z [ERROR]   TeamsPageTest.checkTeamsFilterCount:214->createTeam:102 Team not added
2022-03-04T19:11:30.4466525Z [ERROR]   TeamsPageTest.createTeam:102 Team not added
2022-03-04T19:11:30.4467234Z [ERROR]   TeamsPageTest.editDescription:148->createTeam:102 Team not added
2022-03-04T19:11:30.4483251Z [ERROR]   TeamsPageTest.ownerNameIsConsistentCheck:195->addAsset:166->createTeam:102 Team not added
2022-03-04T19:11:30.4484889Z [ERROR] Errors: 
2022-03-04T19:11:30.4497903Z [ERROR]   CommonTests.overviewLinksAfterTour:317 » Timeout Expected condition failed: wa...
2022-03-04T19:11:30.4498745Z [ERROR]   CommonTests.tourStepSkippingCheck:329 » Timeout Expected condition failed: wai...
2022-03-04T19:11:30.4499483Z [ERROR]   DatabaseServicePageTest.checkIngestionTab:141 » Timeout Expected condition fai...
2022-03-04T19:11:30.4500133Z [ERROR]   MyDataPageTest.checkFollowingTab:260 » NoSuchElement no such element: Unable t...
2022-03-04T19:11:30.4500758Z [ERROR]   RolesPageTest.checkDuplicateRoleName:206 » Timeout Expected condition failed: ...
2022-03-04T19:11:30.4501355Z [ERROR]   RolesPageTest.editDescription:107 » Timeout Expected condition failed: waiting...
2022-03-04T19:11:30.4501991Z [ERROR]   TableDetailsPageTest.checkBreadCrumb:275 » NoSuchElement no such element: Unab...
2022-03-04T19:11:30.4502653Z [ERROR]   TableDetailsPageTest.checkFrequentlyJoinedColumns:320 » Timeout Expected condi...
2022-03-04T19:11:30.4505288Z [ERROR]   TableDetailsPageTest.checkFrequentlyJoinedTables:309 » Timeout Expected condit...
2022-03-04T19:11:30.4506187Z [ERROR]   TagsPageTest.TagUsageCheck:239 » Timeout Expected condition failed: waiting fo...
2022-03-04T19:11:30.4506929Z [ERROR]   TagsPageTest.removeTagWithExistingName:254 » Timeout Expected condition failed...
2022-03-04T19:11:30.4507601Z [ERROR]   TeamsPageTest.teamsWithSameDisplayNameCheck:251 » NoSuchElement no such elemen...
2022-03-04T19:11:30.4508280Z [ERROR]   TopicDetailsPageTest.checkFollow:112 » NoSuchElement no such element: Unable t...
2022-03-04T19:11:30.4508934Z [ERROR]   TopicDetailsPageTest.checkManage:198 » ElementClickIntercepted element click i...
2022-03-04T19:11:30.4509669Z [INFO] 
2022-03-04T19:11:30.4510096Z [ERROR] Tests run: 161, Failures: 16, Errors: 14, Skipped: 0

@vivekratnavel
Copy link
Contributor

I have opened #3158 to fix Selenium test failures

@vivekratnavel vivekratnavel merged commit 1f04306 into main Mar 4, 2022
@vivekratnavel vivekratnavel deleted the feat-request-description branch March 4, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vivekratnavel vivekratnavel vivekratnavel approved these changes

@shahsank3t shahsank3t Awaiting requested review from shahsank3t

@darth-coder00 darth-coder00 Awaiting requested review from darth-coder00

Assignees

@Sachin-chaurasiya Sachin-chaurasiya

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@Sachin-chaurasiya @vivekratnavel @darth-coder00