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.

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

Remove projects from sidebar if there are no projects #1965

Closed
wants to merge 6 commits into from

Conversation

notlmn
Copy link
Contributor

@notlmn notlmn commented Apr 22, 2019

Closes #977
Replaces and closes #1945

This features removes the projects section in the issue and PR sidebar if the user doesn't have any permissions or if there are no projects to select.

@fregante
Copy link
Member

  • safeElementReady doesn't apply to onAjaxedPages because onAjaxedPages waits for dom ready
  • only isPRConversation has the sidebar

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Great solution! It works already, however I'd like to see some caching, like in releases-tab.tsx:

const repoKey = `hide-projects:${repoUrl}`;

@notlmn
Copy link
Contributor Author

notlmn commented Apr 22, 2019

Great solution! It works already, however I'd like to see some caching, like in releases-tab.tsx:

Wouldn't that conflict with organization level projects. What I mean is that if we cache count for each repo and a project is added at organization level, the user might have to wait until the cache expires.

Not saying its a bad idea, but thinking if there would be any problems, because releases is for a single repo, but projects work at both repo and organization level!? 🤔🤷‍♂️

@fregante
Copy link
Member

That raises another question: is this piece of code correctly counting the projects in organizations as well?

And also: do closed projects appear in the dropdown?

the user might have to wait until the cache expires

That's kinda the issue with all caches. The workaround would be to have a short cache (1 day)

@fregante
Copy link
Member

fregante commented Apr 22, 2019

Alternatively, instead of hiding the Projects, we could condense the sidebar:

hide all the Empty messages from the sidebarhide all the empty sections when the current user can't do anything about them

Related feature request to keep in mind: #421

@notlmn
Copy link
Contributor Author

notlmn commented Apr 22, 2019

That raises another question: is this piece of code correctly counting the projects in organizations as well?

Yup, the partially loaded DOM contains item listing for both repository and organization projects combined in their own containers. Here we are counting these combined projects (example).


And also: do closed projects appear in the dropdown?

Just tested it, no they don't. 🎉


That's kinda the issue with all caches. The workaround would be to have a short cache (1 day)

I'll see what I can do with caching, might take time though.

@notlmn
Copy link
Contributor Author

notlmn commented Apr 22, 2019

Alternatively, instead of hiding the Projects, we could condense the sidebar

Already thought of generalizing this feature hide all the empty sidebar stuff, but stopped after looking at "Assignees", that looks like a useful thing to have even when it doesn't have anything to show, or maybe that's just me.

Open to other thoughts too! 😉

(Edit: "Assignees" and "Labels" almost always exist, but "Projects" and "Milestones" might not)

@fregante
Copy link
Member

fregante commented May 1, 2019

@sindresorhus agreed that this is the way to go, clean-discussion-sidebar: #1965 (comment)

  1. Hide empty sections for regular users
  2. Hide "None yet", "No milestone", etc for collaborators
  3. Move "Assign yourself" next to "Assignees" header, if empty

And

  1. I would go as far as hiding the section header for Labels and Notifications:

This would reduce noise considerably on "empty" discussions like parcel-bundler/parcel#2840

BeforeAfter

Test on Parcel PRs too, they seem to use all features: https://github.com/parcel-bundler/parcel/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc

@fregante
Copy link
Member

fregante commented May 3, 2019

That's not what I meant 😅

  1. Hide "None yet", "No milestone", etc for collaborators

The section should stay there for collaborators, it should only hide that extra text. This feature should not require any fetches

Refer to this screenshot (+ improved spacing and point 3 and 4 of my last comment)

@notlmn
Copy link
Contributor Author

notlmn commented May 3, 2019

The section should stay there for collaborators, it should only hide that extra text. This feature should not require any fetches

That does not actually align with what #977 and #421 sounds, but can be manageable. Will update in a bit.

@notlmn
Copy link
Contributor Author

notlmn commented May 3, 2019

Screenshots

Repo with no permissions

PR to some repo

Before After
1-before 1-after

Issue on some repo

Before After
2-before 2-after

Repo with permissions

Assigned issue with projects

Before After
3-before 3-after

Non-assigned issue with projects

Before After
4-before 4-after

  1. Hide empty sections for regular users

Done.


  1. Move "Assign yourself" next to "Assignees" header, if empty

That's not possible, the header itself is a <details> element, can't put button inside another interactive element. Can position it absolutely, but I don't see the point in that.

@notlmn
Copy link
Contributor Author

notlmn commented May 3, 2019

  1. I would go as far as hiding the section header for Labels and Notifications

I don't know good this looks like. Is this okay?
save

@fregante
Copy link
Member

fregante commented May 3, 2019

The section should stay there for collaborators, it should only hide that extra text. This feature should not require any fetches

This

It should not depend on whether there are projects and milestones at all. Only if none are assigned.

Refer to this, step by step: #1965 (comment) and the first screenshot here: #1965 (comment)

@fregante fregante mentioned this pull request May 5, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Remove Projects from the sidebar if there are no Projects
2 participants