Skip to content

Conversation

kelvinator07
Copy link
Contributor

Closes #836

Replaces #848

Description

This feature adds a new optional field for the description of each network along with the network name.

Steps to Test

  1. Create a network and add the name and description (100 characters limit) in the fields provided.
  2. View the network name and description when you navigate to NetworkView.

Screenshots

Home view

Screenshot 2024-08-14 at 21 06 24

NetworkView

Screenshot 2024-08-14 at 21 07 22

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (028055c) to head (d3e0857).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #978   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          151       151           
  Lines         5500      5509    +9     
  Branches      1105      1066   -39     
=========================================
+ Hits          5500      5509    +9     

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

@jamaljsr jamaljsr self-requested a review August 15, 2024 17:45
Copy link
Owner

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this. Overall everything looks great. I just have a few small suggestions for improvement.

image

  • since there are now multiple fields, it would be good to have placeholders on them so it's clear what each field if for.
  • I found an existing bug while testing this. If I don't enter a name, the error message is not displayed properly because the lang key doesn't exist in the en-US.json file. Would you mind fixing this here in this PR?

image

  • I feel like the description is taking up extra space and doesn't really need to be displayed at all times. I like the tooltip on the main Networks screen. Can you use that info icon on this screen as well?

@kelvinator07
Copy link
Contributor Author

Hi @jamaljsr Thanks for your review. I'll look into the bug you mentioned and revert to the tooltip.

@kelvinator07 kelvinator07 requested a review from jamaljsr August 27, 2024 23:48
@kelvinator07
Copy link
Contributor Author

Hi @jamaljsr Updated the PR with the bug fix and your suggestion. Kindly review. Thanks

Copy link
Owner

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. This looks great now!

@jamaljsr jamaljsr merged commit aa8ba74 into jamaljsr:master Aug 28, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: add a description field for each network
2 participants