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

🐾 Enhance the DetailsItem component to enable both hyperlink and edit #1122

Merged

Conversation

sgratch
Copy link
Collaborator

@sgratch sgratch commented Apr 18, 2024

References:
#1106
#1124

Up till now the DetailsItem component supported 2 modes:

  1. A non editable content (w/o hyperlink).
  2. An editable content while both content and pencil icon are an inline button for triggering the onEdit callback.

This PR enhances the DetailsItem component by replacing the editable mode style (2) such that only the pencil icon is an inline button for triggering the onEdit callback, while the field's context element is now separated from the edit button and can be used as read only text element, a hyperlink button etc.

Screencast of 2 examples for the editable mode usage (readable text and hyperlink)

Screencast.from.2024-04-24.17-44-09.webm

Future enhancement:

We can consider adding a 'Edit' label next to the pencil icon:

pencil icon with a label

Screenshot from 2024-04-18 21-33-05

@sgratch sgratch force-pushed the enhance-DetailsItem-to-support-hyperlink-and-edit branch from 4ca8417 to 06917ca Compare April 18, 2024 20:02
@sgratch sgratch requested a review from yaacov April 18, 2024 20:03
@yaacov
Copy link
Member

yaacov commented Apr 19, 2024

Nice,
The idea is to be able to support external links in the "value" part of the item, makes sense to me.

my concern is that we will have two identical items that will behave differntly without visual indication, one will have "value"+"pencil" where the "value" will work as an "edit button", and in the other the "value" will work as "external link". Can you think of another way to have "value" as a "external link" while keeping only one behavior, maybe switching to only support the pencilOnlyButton mode ?

@yaacov yaacov added the enhancement Categorizes issue or PR as related to a new feature. label Apr 19, 2024
@yaacov yaacov added this to the 2.7.0 milestone Apr 19, 2024
@yaacov yaacov changed the title Enhance the DetailsItem component to enable both hyperlink and edit 🐾 Enhance the DetailsItem component to enable both hyperlink and edit Apr 19, 2024
@yaacov
Copy link
Member

yaacov commented Apr 19, 2024

Ref: #1124

@sgratch
Copy link
Collaborator Author

sgratch commented Apr 21, 2024

Another suggestion for enhancement:

Why not adding the console's commonly used Annotations field to the providers details? This will enable the user to easily add annotations like for the external provider console link. E.g.
Screenshot from 2024-04-21 17-40-27

We already support the forklift.konveyor.io/defaultTransferNetwork annotation and might support more in the future so it seems relevant to add this field

@sgratch
Copy link
Collaborator Author

sgratch commented Apr 21, 2024

my concern is that we will have two identical items that will behave differntly without visual indication, one will have "value"+"pencil" where the "value" will work as an "edit button", and in the other the "value" will work as "external link". Can you think of another way to have "value" as a "external link" while keeping only one behavior, maybe switching to only support the pencilOnlyButton mode ?

The reason I used this "combined" solution is since other details pages used the same style. For example, check the Secret details page (Labels and Annotations are both editable fields, each label, if added, can be clicked as well):

Screenshot from 2024-04-21 17-29-51

Anyway, I agree that it's confusing and also suggested in main comment above to consider adding an 'Edit' label next to the pencil icon, as done for the Console's Labels field. Maybe it's even better to implement that for all editable fields in the provider details page. Something like this:

Screencast.from.2024-04-21.21-58-45.webm

WDYT?

@yaacov
Copy link
Member

yaacov commented Apr 24, 2024

Another suggestion for enhancement:
Why not adding the console's commonly used Annotations field to the providers details? This will enable the user to easily add annotations like for the external provider console link

That is an excellent suggestion and in line with the product plan, we can add an enhancement feature and prioritize according to our capacity.

@yaacov
Copy link
Member

yaacov commented Apr 24, 2024

consider adding an 'Edit' label next to the pencil icon, as done for the Console's Labels field. Maybe it's even better to implement that for all editable fields in the provider details page

Adding Edit next to each Pencil icon, is another enhancement we can consider, my concern with adding Edit always is that is does not align with the current console design, usually in the console they add Pencil when it's a one-item view and the Pencil is placed next to the item the user will edit, and when the view is large, like we have in Labels and Annotations they put the item in a box, and put Edit + Icon on the top right corner of this box.

Copy link
Member

@yaacov yaacov left a comment

Choose a reason for hiding this comment

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

Please, make only one way of rendering the component, always separating the content from the icon button.

See: #1122 (comment)

@sgratch
Copy link
Collaborator Author

sgratch commented Apr 24, 2024

consider adding an 'Edit' label next to the pencil icon, as done for the Console's Labels field. Maybe it's even better to implement that for all editable fields in the provider details page

Adding Edit next to each Pencil icon, is another enhancement we can consider, my concern with adding Edit always is that is does not align with the current console design, usually in the console they add Pencil when it's a one-item view and the Pencil is placed next to the item the user will edit, and when the view is large, like we have in Labels and Annotations they put the item in a box, and put Edit + Icon on the top right corner of this box.

Yes I agree. In general, I think it's confusing for the user to use 2 kind of edit button styles within the same page.
I'll add only the pencil icon style and we can enhance it later.

@sgratch sgratch force-pushed the enhance-DetailsItem-to-support-hyperlink-and-edit branch from 06917ca to 8cf7da4 Compare April 24, 2024 14:13
{content} <Pencil className="forklift-page-details-edit-pencil" />
</DescriptionListDescription>
</Button>
export const EditableContentPencilOnlyButton: React.FC<{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const EditableContentPencilOnlyButton: React.FC<{
export const EditableContentButton: React.FC<{

Copy link
Member

Choose a reason for hiding this comment

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

no need to change the element name, we only have one EditableContentButton

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually like to name the element with it's style, so that we will be able to add other styles in the future (e.g. w/o edit label etc) and since the console itself includes few edit styles already.
But if you think it's redundant then ok.

Copy link
Member

Choose a reason for hiding this comment

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

But if you think it's redundant then ok.

yes, I do :-)

* @param {ReactNode} content - The content of the button.
* @param {Function} onEdit - Function to be called when the button is clicked.
* @param {ReactNode} content - The field's content element.
* @param {Function} onEdit - Function to be called when the button with the pencil icon is clicked.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {Function} onEdit - Function to be called when the button with the pencil icon is clicked.
* @param {Function} onEdit - Function to be called when the button is clicked.

we only have one button

Reference: kubev2v#1106

Up till now the DetailsItem component could support 2 modes:
1. A non editable content (w/o hyperlink).
2. An editable content while both content and pencil icon are inline button for
triggering the onEdit callback.

This PR enhances the DetailsItem component to replace the editable mode (2) style such that
the field context is an element (can be a hyperlink button for example) and only the pencil icon
is an inline button for triggering the onEdit callback.

We can consider adding a 'Edit' label next to the pencil icon.

Signed-off-by: Sharon Gratch <sgratch@redhat.com>
@sgratch sgratch force-pushed the enhance-DetailsItem-to-support-hyperlink-and-edit branch from 8cf7da4 to 1a86fa7 Compare April 24, 2024 14:38
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@sgratch
Copy link
Collaborator Author

sgratch commented Apr 24, 2024

Updated the main comment with a screencast.

@yaacov yaacov merged commit 0505233 into kubev2v:main Apr 24, 2024
7 checks passed
@yaacov yaacov modified the milestones: 2.7.0, 2.6.1 May 2, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants