-
Notifications
You must be signed in to change notification settings - Fork 357
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.
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
feat(ClipboardCopy): 9712 allow alternative title #9713
base: v5
Are you sure you want to change the base?
feat(ClipboardCopy): 9712 allow alternative title #9713
Conversation
Preview: https://patternfly-react-pr-9713.surge.sh A11y report: https://patternfly-react-pr-9713-a11y.surge.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same reservations as Austin noted in the issue, in that it becomes a little unclear what will actually get copied (unless it's explicitly stated in the title text something like "Yadda yadda (copyable content below)". Right now it's a little apparent since this new example doesn't have any truncation vs the other examples truncating the input content, making it clear the expanded content is the full text, but we may not be able to rely on that depending what title
is passed in:
Something like a "label" above the clipboard copy could maybe work (just bold and some bottom padding to illustrate):
Using a span instead of input could be another option, though not sure if it's really distinct enough still, even with toying with the background color to remove the read only styling (possible plus is this won't truncate, it'll just wrap the title text so at least then it's more distinct from a expanded readonly variant):
Something possibly along the lines of what Austin mention in the issue (more GitHub-like), basically just removing the bottom border and the readonly styling:
@thatblindgeye - seems like we need some design input here? Personally I think the "label above" example you showed makes a lot of sense, and wouldn't even require any change from us. I'd want to make sure and run that idea across the product that brought up this issue though. |
@andrew-ronaldson @mmenestr Pinging for the design question above Also needs a rebase |
Just to make sure I understand correctly, the suggestion is that instead of showing a truncated version of what you want to copy, you'd want to have some sort of label to say "hey, expand this to see what would be copied"? |
@mmenestr That was the original suggestion in the issue, yeah, which Dallas' second image above shows. There was some comments in the original issue with some worries about going that route (confusion as to what is actually getting copied). Dallas' first screenshot was one alternative mentioned in the original issue (there's also another alternative in there that would look similar to GitHub code blocks). |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are adding the input label version for alternate text as an variation to this component I think this is good to go.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Closes #9712
https://patternfly-react-pr-9713.surge.sh/components/clipboard-copy#expanded-with-alternate-title
unsure if my verbiage/naming was the best.
setting a title will force the title itself to be readOnly (disabled textinput), but a user can still edit the text that gets copied if there is expandable content for them to do it in the text area, unless
isReadOnly
is specified in which case they wouldn't be able to edit either place.