search

plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 574 forks source link

It should be possible to open linked items in a new tab #4580

Open tiberiuichim opened 1 year ago

tiberiuichim commented 1 year ago

https://github.com/plone/volto/blob/d3865b3ac9e5c1213058373e6ecf5422060d7ab0/src/components/manage/Widgets/ObjectBrowserWidget.jsx#L110-L143

The trigger should be <a> link, so that it can be previewed in a new tab.

ishk9 commented 1 year ago

@tiberiuichim Hey! Can I work on this?

tiberiuichim commented 1 year ago

@ishaan9006 Yes, please do

falgunmpatel commented 1 year ago

@tiberiuichim Were my changes any helpful? Or I cteated any blunder? Would anyone help me understand?

falgunmpatel commented 1 year ago

@tiberiuichim Were my changes any helpful? Or I cteated any blunder? Would anyone help me understand?

@stevepiercy @sneridagh @nileshgulia1 @JeffersonBledsoe

tiberiuichim commented 1 year ago

@falgunmpatel I think it's fine

falgunmpatel commented 1 year ago

@tiberiuichim Thanks for getting back to me so quickly! Your answer was exactly what I needed.

JeffersonBledsoe commented 1 year ago

@tiberiuichim It would be nice if we indicated to the user that links opened in a new tab. While not a WCAG/ ATAG violation (I don't think), it is noted as best practise. Should this be a global option or a per-link/ schema (and thus part of this PR) option do you think? This could also be visually hidden text

stevepiercy commented 1 year ago

@tiberiuichim It would be nice if we indicated to the user that links opened in a new tab.

Uh, I think you mean to say:

"When links open in a new tab or window, the link should indicate that."

The way you phrased it makes it sound like all links open in a new tab, and we should indicate that. If that were true, I would rage quit Plone as being UX hostile.

@falgunmpatel it appears that you opened two PRs with similar solutions. Why? You should have only one PR per solution, and close obsolete ones. Please advise so core developers do not waste their time reviewing the obsolete PR. Thank you.

falgunmpatel commented 1 year ago

@stevepiercy I think I have only one PR open. Sorry if I caused any trouble as a newbie.

stevepiercy commented 1 year ago

@falgunmpatel there are five PRs that reference this issue, three of which are closed, two of which are still open.

This is sloppy work and wastes core developer time, trying to figure out what you are proposing for review. Please clean this up.

falgunmpatel commented 1 year ago

@stevepiercy #4591 is not my pull request. @ishaan9006 created it. Can I remove it and how?

JeffersonBledsoe commented 1 year ago

Uh, I think you mean to say:

"When links open in a new tab or window, the link should indicate that."

Yes, that's what I meant, I was leaning too much on the context of this issue. Thanks for the clarification Steve!

Thinking about my comment some more: displaying Opens in new tab/ window on each item in the value list for an object browser widget would not be a nice UI visually and could become fatiguing for screen-reader users. How about some text in the description of the widget to let users know they can click on the value to preview the content?

tiberiuichim commented 1 year ago

I think this is the kind of "deep knowledge" of the framework that editors gain while using the system.

In principle, if the text inside label is formatted as a link (maybe an underline), then the users don't need other cues that it's a link. Then, the fact that it opens in a new window it's just to protect them from mistakes. No need to burden them with this knowledge, it's just intrinsec.

stevepiercy commented 1 year ago

@falgunmpatel sorry, we've recently been flooded with poor quality PRs getting opened and closed rapidly, and irrelevant comments in issues, which causes me not to be as careful as I usually am to sort through the lot.

Your PR is preferable because it complies with contributor guidelines and has a news item. @ishaan9006's does not. Additionally your code is cleaner. It needs to be updated by merging master into it.

There is no need to @ team members in the original issue. When you submit a PR, it is a good practice to request a review by selecting users from the Reviewers menu, which will send an email notification to those reviewers. Always select the person who created the issue, and optionally others who may have an interest in the solution you propose. When creating a PR, in its description include "Closes #IssueNumber". That creates a link to the original issue number, and when the PR is merged, it will automatically close the original issue.

falgunmpatel commented 1 year ago

@stevepiercy @tiberiuichim Since I was removed from team "developers" and added to team "contributors", i no more have rights to add specific reviewers to my pull requests. Therefore I have to @ you. I hope you understand.

Also I opened a latest pull request and closed my previous one updating the master branch into it. Please review it.

stevepiercy commented 1 year ago

@falgunmpatel OK, I will let the admin team know about your lack of access to request reviews. In that case, @-ing someone from the pull request, not the issue, would be preferred, so that the recipient can click the link in the email notification to go directly to the pull request and review it.

With your current group membership, do you have access to assign an issue or pull request to yourself?

Also to update your pull request, at the end, you might see a notice from GitHub:

Screen Shot 2023-03-24 at 5 06 05 PM

This branch is out-of-date with the base branch

Merge the latest changes from master into this branch.

You would then click the Update branch button. That's all. There is no need to go through closing, reopening, or opening new PRs. That's too much. Just work on the one PR, else you will drive us 🤪 trying to follow your intent. If you don't know how to do that, we can point you to docs on GitHub to give you guidance.

I'm in the process of updating our documentation for onboarding, and I might have to update it with these bits and pieces. Please let me know. Thank you!

falgunmpatel commented 1 year ago

@stevepiercy I do not have rights to assign myself to the issue now, I assigned myself when i was in developers team.

And sorry for the number of PRs I opened. I will improve myself soon enough and never let you down.

falgunmpatel commented 1 year ago

@stevepiercy I am not shown any option to update this branch with master.

Screenshot 2023-03-25 at 12 23 34 PM
stevepiercy commented 1 year ago

I can't tell which PR, so I assume it is this one: https://github.com/plone/volto/pull/4600. It is best to comment in the PR in the future. Here is what I see:

Screen Shot 2023-03-25 at 12 24 00 AM

This might be yet another permissions thing. Please verify that I am looking at the correct PR. Thank you!

falgunmpatel commented 1 year ago

@stevepiercy #4600 is the correct PR

stevepiercy commented 1 year ago

This is yet another permission issue. I've notified the team. I've also clicked the Update branch button to run the GitHub Actions.

fredvd commented 1 year ago

The permission issues discussed here follow from first creating the branch on the official repo and then loosing the permissions. I've temporarily granted @falgunmpatel write permission so we can finish the PR and close this issue.