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(web): some style edited #1529

Closed
wants to merge 3 commits into from
Closed

fix(web): some style edited #1529

wants to merge 3 commits into from

Conversation

trainto
Copy link
Contributor

@trainto trainto commented Nov 19, 2020

bb4ee9c

  • When hover icons on menu, border looks a bit rough, so some border-radius added.

8bed360

  • When it comes to dark mode, --color-fg-primary is white, but popups background is light grey, so font is hard to read.
    --color-fg-on-popup added, and applied to overflow menu, and dialogs.

@ckerr
Copy link
Member

ckerr commented Nov 20, 2020

...oog. I just made that comment in #1530 about "maybe transmission-app.js shouldn't be in the repo, since it's a generated file" and then came straight into here and see that conflict. Yeah, we're going to have conflicts in that file pretty much every time there are two in-flight web client PRs, aren't we.

PR itself looks good. I like these improvements & I appreciate the feedback on the UI redesign. 👍

@trainto
Copy link
Contributor Author

trainto commented Nov 21, 2020

@ckerr I appreciate your feedback.

...oog. I just made that comment in #1530 about "maybe transmission-app.js shouldn't be in the repo, since it's a generated file" and then came straight into here and see that conflict. Yeah, we're going to have conflicts in that file pretty much every time there are two in-flight web client PRs, aren't we.

I agree with you. I think build result should not be tracked by git and saved in the repo. Instead maybe make install process includes 'npm run build' execution before installing web client.(Never mind, This will force developers who are not involved with web dev. to install node, npm and etc. which is inefficient.)

Anyhow, if you want me to eliminate transmission-app.js from this PR, I'll make the change.

@egroenen
Copy link

egroenen commented Jan 3, 2021

@trainto Perhaps the transmission-app.js file should stay in the repo, however, it should be marked as being auto-generated and treated as a binary to prevent the merge conflicts.

We can do this by creating a .gitattributes file with the contents:

transmission-app.js    -diff -merge
transmission-app.js    linguist-generated=true

You may well ask what the conflict policy will be now, same as for binaries, the newest will win, which I believe is what we want here.

Reference: Managing generated files in GitHub

@trainto
Copy link
Contributor Author

trainto commented Jan 5, 2021

@egroenen Thank you for sharing this info. I didn't know there is this kind of functionality in Git. I'll try to add .gitattributes and update this PR.

Since transmission-app.js is a generated file,
should be treated as a binary to avoid conflict.
@trainto
Copy link
Contributor Author

trainto commented Jan 5, 2021

@ckerr Happy new year!!
As @egroenen suggested, I added .gitattributes to web directory, to avoid conflict of transmission-app.js and treat it as a binary.
Maybe you could take a look on this?

@ckerr
Copy link
Member

ckerr commented Oct 13, 2021

I know how long this PR has been in limbo and am going through PRs to try to reduce the project's PR backlog.

@trainto if you'd still like to see this PR land, please update it to resolve the merge conflicts and I'll merge it. Thanks!

@ckerr ckerr added scope:web needs update The PR has needs to be updated by the submitter labels Oct 13, 2021
@ckerr ckerr mentioned this pull request Feb 11, 2022
@ckerr
Copy link
Member

ckerr commented Feb 11, 2022

Superseded by #2609. I resolved the merge conflicts, @trainto gets author credit for the changes. Sorry for taking so long to merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs update The PR has needs to be updated by the submitter scope:web
Development

Successfully merging this pull request may close these issues.

None yet

3 participants