Skip to content

Improve cleanup-repo-filelist-actions’s reliability #3531

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.

Sign up for GitHub

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

Jump to bottom
Closed
kidonng wants to merge 10 commits into from
Closed

Improve cleanup-repo-filelist-actions’s reliability #3531

kidonng wants to merge 10 commits into from

Conversation

@kidonng
Copy link
Member

@kidonng kidonng commented Sep 5, 2020 •
edited
Loading

@fregante fregante changed the title Reimplement #3517 Improve cleanup-repo-filelist-actions’s reliability Sep 5, 2020
@fregante fregante added the bug label Sep 5, 2020
@kidonng kidonng marked this pull request as ready for review September 8, 2020 22:46
@fregante fregante closed this Sep 9, 2020
@fregante fregante reopened this Sep 25, 2020
@fregante
Copy link
Member

fregante commented Sep 25, 2020

This will also need a rgh-clean-actions class as described in #3094

@fregante fregante marked this pull request as draft September 25, 2020 01:14
@fregante fregante mentioned this pull request Sep 25, 2020
Closed
@kidonng kidonng force-pushed the cleanup-repo-filelist-actions branch from 1a3decc to 5305582 Compare October 15, 2020 01:49
@kidonng

This comment has been minimized.

Sign in to view
@kidonng kidonng marked this pull request as ready for review October 16, 2020 09:44
@yakov116

This comment has been minimized.

Sign in to view
@kidonng

This comment has been minimized.

Sign in to view
@kidonng
Copy link
Member Author

kidonng commented Oct 16, 2020 •
edited
Loading

Caused by https://github.com/sindresorhus/refined-github/pull/3646/files#diff-5ba5120afe524f2d5eed502b044996a1db29ba24221b41edee322940746bf553R20-R21

Add button doesn't exist on files page

@yakov116
Copy link
Member

yakov116 commented Oct 16, 2020

That's why we had 3 approvals 🤣.

@kidonng

This comment has been minimized.

Sign in to view
@kidonng
Copy link
Member Author

kidonng commented Oct 16, 2020 •
edited
Loading

I would also like to take this opportunity to rename this feature to clean-repo-filelist-actions (the name was originally suggested in #3405 (comment)) since we have many clean-* features.

@yakov116
Copy link
Member

yakov116 commented Oct 16, 2020

I would also like to take this opportunity to rename this feature to clean-repo-filelist-actions since we have many clean-* features.

Please do #3514 (comment)

@kidonng
Copy link
Member Author

kidonng commented Oct 16, 2020 •
edited
Loading

I also turned the "Download" text of download-folder-button into a tooltip, as mentioned in #3405 (comment). This makes it more consistence with other buttons

image

image

But if it doesn't suit in this PR I can extract it

fregante
fregante reviewed Oct 16, 2020
View reviewed changes
yakov116
yakov116 reviewed Oct 18, 2020
View reviewed changes
@yakov116
Copy link
Member

yakov116 commented Oct 20, 2020 

refined-github.js:1627 Uncaught TypeError: Cannot read property 'replaceWith' of null
    at add (refined-github.js:1627)
    at runAdd (refined-github.js:17277)
    at applyChanges (refined-github.js:17193)
    at SelectorObserver.addRootNodes (refined-github.js:17713)
    at Array.processBatchQueue (refined-github.js:17134)
    at MutationObserver.handleMutations (refined-github.js:17168)

On https://github.com/sindresorhus/refined-github/tree/master/source

@yakov116 yakov116 mentioned this pull request Oct 21, 2020
Closed
ocarreterom
ocarreterom reviewed Oct 21, 2020
View reviewed changes
source/features/clean-repo-filelist-actions.tsx

observe('get-repo summary:not(.rgh-clean-actions)', {
add(button) {
button.classList.add('tooltipped', 'tooltipped-ne', 'rgh-clean-actions');
Copy link

@ocarreterom ocarreterom Oct 21, 2020

Choose a reason for hiding this comment

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

Could you check if click outside to close works?. That's the reason I moved that classes from <summary> to <details> here.

ocarreterom
ocarreterom reviewed Oct 21, 2020
View reviewed changes
source/features/clean-repo-filelist-actions.tsx
// Exclude logged out, mobile or file pages
const addButtonText = searchButton.nextElementSibling?.querySelector('.d-md-flex');
if (addButtonText) {
addButtonText.parentElement!.classList.add('tooltipped', 'tooltipped-ne');
Copy link

@ocarreterom ocarreterom Oct 21, 2020

Choose a reason for hiding this comment

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

👇 (if parentElement is a <summary>)

Copy link
Member

@fregante fregante Oct 25, 2020

Choose a reason for hiding this comment

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

In what case it's not a summary? My understanding is that it's a summary when it's a dropdown, and it always is.

@fregante fregante mentioned this pull request Oct 23, 2020
Closed
fregante
fregante reviewed Oct 25, 2020
View reviewed changes
readme.md
- [](# "clone-branch") [Clone a branch from the branches list.](https://user-images.githubusercontent.com/16872793/76802029-2a020500-67ad-11ea-95dc-bee1b1352976.png)
- [](# "fork-source-link-same-view") [Points the “Forked from user/repository” link to current folder or file in the upstream repository.](https://user-images.githubusercontent.com/1402241/84795784-3722d000-aff8-11ea-9b34-97c01acf4fd4.png)
- [](# "cleanup-repo-filelist-actions") [Replaces the labels of some simple buttons on repository filelists with icons, making them take less space.](https://user-images.githubusercontent.com/44045911/88551471-7a3f7c80-d055-11ea-82f1-c558b7871824.png)
- [](# "clean-repo-filelist-actions") [Replaces the labels of some simple buttons on repository filelists with icons, making them take less space.](https://user-images.githubusercontent.com/44045911/96283396-bbb94580-100e-11eb-9480-17954bb3a811.png)
Copy link
Member

@fregante fregante Oct 25, 2020

Choose a reason for hiding this comment

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

Some padding is ok, but this is excessive. Can you avoid it and instead use higher-resolution screenshots? Zoom the page at 200% if possible before taking a screenshot

@fregante
Copy link
Member

fregante commented Oct 25, 2020 •
edited
Loading

There are 2 bugs related to this feature, can you ensure this PR fixes them and include them as Fixes #3675 and fixes #3685?

#3685
#3675

@fregante fregante marked this pull request as draft October 31, 2020 01:01
@fregante
Copy link
Member

fregante commented Nov 1, 2020

Closing this since it’s inactive to allow anyone to possibly work on it. Feel free to open a new PR based on this code.

@fregante fregante closed this Nov 1, 2020
This was referenced Feb 14, 2021
Closed
Merged
@cheap-glitch cheap-glitch mentioned this pull request Feb 18, 2021
Merged
@kidonng kidonng deleted the cleanup-repo-filelist-actions branch August 2, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fregante fregante fregante left review comments

@yakov116 yakov116 yakov116 left review comments

+1 more reviewer

@ocarreterom ocarreterom ocarreterom left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kidonng @fregante @yakov116 @ocarreterom