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

Fix set-default-repositories-type-to-sources on project views #5949

Merged
merged 4 commits into from
Sep 8, 2022

Conversation

chcamiloam
Copy link
Contributor

Closes #5940

Test URLs

https://github.com/users/janpio/projects/3/views/1

Screenshot

Screen Shot 2022-09-07 at 14 50 35

Description

The error was produces by a selector trying to find an item that doesn't exists in the page.

Changes

The method was doing reference to the user's profile dropdown using the :last-child selector, looks like later a new item was added turning the user's profile item in the second last item. This PR change the selector to select the profile dropdown again.

Screen Shot 2022-09-07 at 15 58 14

@fregante
Copy link
Member

fregante commented Sep 7, 2022

Thank you for the PR! Doesn't this break it on every other page where the current selector works?

@chcamiloam
Copy link
Contributor Author

chcamiloam commented Sep 7, 2022

I think we need a validation to only use the new selector in this page, it is not breaking anything but is generating console errors in other pages.
I would like to use Github-url-detection to track this page too. I sent this PR to that repo refined-github/github-url-detection#149 To add the projects/views route to the detection package. Once it is merged I'll send the update to this PR, if you think there is another way to add the validation I'll happy to try it.

@fregante
Copy link
Member

fregante commented Sep 7, 2022

I think it doesn't need a per-page selector. We can fix it by using a different selector that doesn't involve nth-child.

@fregante
Copy link
Member

fregante commented Sep 7, 2022

Actually I think this whole file can be replaced by our selector-observer.ts (not to be confused with the library with we also still use)

Instead of having a custom onProfileDropdown event, we can use the observer to target whichever link we need in the drop-down.

Would you like to give that a try? If not, I will.

Example:

async function profileDropdown(): Promise<void> {
await onProfileDropdownLoad();
addSourceTypeToLink(select('.header-nav-current-user ~ a[href$="tab=repositories"]')!); // "Your repositories" in header dropdown
}

should be something like:

observe('.header-nav-current-user ~ a[href$="tab=repositories"]', addSourceTypeToLink)

@chcamiloam
Copy link
Contributor Author

Let me check how it works, I'll try with the observer, also I was checking other options, looks like the item we are looking for have this unique mix of clases .position-relative.Header-item.mr-0 at it works perfectly, but let me try the observer

@fregante
Copy link
Member

fregante commented Sep 7, 2022

Sure. Once you get it working, you can refer to how it's generally used. For example:

  • it should accept a {signal} parameter (see how that works in other features the use selector-observer.ts)
  • it should not use onetime in the definition (which it currently does)

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.

Fantastic!

@fregante fregante added the bug label Sep 8, 2022
@fregante fregante changed the title Add correct selector to profile dropdown Fix set-default-repositories-type-to-sources on project views Sep 8, 2022
@chcamiloam
Copy link
Contributor Author

Type=source is added to the URL and no errors in the console.

Screen Shot 2022-09-08 at 10 00 15

We could add some pre-commits here 😅

@fregante
Copy link
Member

fregante commented Sep 8, 2022

We could add some pre-commits here 😅

Linting and testing is slow, post-commits are better in this case. 😜 You wouldn't want to run the whole suite every time you make a single commit

@fregante fregante merged commit cbd4f17 into refined-github:main Sep 8, 2022
@fregante
Copy link
Member

fregante commented Sep 8, 2022

Thank you for your contribution!

@janpio
Copy link
Contributor

janpio commented Sep 8, 2022

Thank you indeed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants