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

Avoid duplicate upload button in minimize-upload-bar #3724

Merged
merged 5 commits into from
Nov 16, 2020

Conversation

ts-ke
Copy link
Contributor

@ts-ke ts-ke commented Nov 14, 2020

  1. LINKED ISSUES:
    Closes Reuse GitHub’s native upload button in minimize-upload-bar #3714

  2. TEST URLS:
    Avoid duplicate upload button in minimize-upload-bar #3724

  3. SCREENSHOT:
    edit: update screenshots
    image for readme:

Screen Shot 2020-11-14 at 4 03 56 pm

@ts-ke ts-ke marked this pull request as draft November 14, 2020 02:55
@ts-ke ts-ke marked this pull request as ready for review November 14, 2020 02:56
@fregante
Copy link
Member

fregante commented Nov 14, 2020

Thanks for the PR!

Both buttons do the same thing, so our button should also be removed.

Additionally, this should be part of minimize-upload-bar and not just an addition to Refined GitHub

@ts-ke
Copy link
Contributor Author

ts-ke commented Nov 14, 2020

@fregante sorry for misunderstanding. updated.

@fregante
Copy link
Member

Much better, thanks!

From what I see, this feature can be simplified further:

  1. The js file should only add a class on body in the init function
  2. The CSS file might need to be updated to expect the class on the body element and then include form in each selector (but ideally a more specific class than that, considering that this applies to a specific widget and not just any form)

@fregante
Copy link
Member

About 2: the previous selectors probably don’t need to be updated at all

@ts-ke
Copy link
Contributor Author

ts-ke commented Nov 14, 2020

  1. thx. I missed that.
  2. I find a class name candidate, js-previewable-comment-form. I have checked "new PR comments", "edit PR comments", "inline-code comments" and "review comments". They seem to be ok. That being said, I don't know all the forms affected by the previous selector so I may have missed a few. So, I cant ensure the new behavior will match to old one. Is it ok?

@fregante
Copy link
Member

Our class needs to be added on body.

Then that form class needs to be used to select the form in your new selector.

The number of forms changes on the page, so the loop will not include all of them.

@ts-ke
Copy link
Contributor Author

ts-ke commented Nov 14, 2020

@fregante Thank you for reviewing. I have tried to address the problem. Can you check if I have understood correctly?

@fregante
Copy link
Member

Looks about right!

@fregante fregante changed the title Show upload image button in 'rgh-minimize-upload-bar' Avoid duplicate upload button in 'rgh-minimize-upload-bar' Nov 15, 2020
@fregante fregante added the bug label Nov 15, 2020
.rgh-minimize-upload-bar .js-previewable-comment-form label[aria-label='Attach an image'] {
display: block !important;
padding: 4px !important;
margin: 0;
Copy link
Member

Choose a reason for hiding this comment

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

This has no effect and is not necessary

Suggested change
margin: 0;

@fregante fregante merged commit dba3b01 into refined-github:master Nov 16, 2020
@fregante fregante changed the title Avoid duplicate upload button in 'rgh-minimize-upload-bar' Avoid duplicate upload button in minimize-upload-bar Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Reuse GitHub’s native upload button in minimize-upload-bar
2 participants