Skip to content

Conversation

@tquetano-r7
Copy link
Contributor

@tquetano-r7 tquetano-r7 commented Jul 10, 2018

  • Make permissions more specific for security reasons
  • Modify colors in style to work with both standard and custom (through Stylish / Stylus) styles

Sorry for the sudden nature of this PR, I just figured it was easier to make the small changes and provide the PR than to discuss it and then humbly request to do the PR. Basically, this PR improves the security of the permissions, and also allows more flexible styling with things like Stylus.

* Make permissions more specific for security reasons
* Modify colors in style to work with both standard and custom (through Stylish / Stylus) styles
Copy link
Owner

@picheli20 picheli20 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the PR 😄

I just don't like to remove the border, maybe add border-radius, it will make look a bit better. Other solution that I see is add .file as well where we are using . gct-file-tree, so we use the proper gh website styles.

app/style.js Outdated
z-index: 28;
width: 280px;
border: 1px solid #ddd;
border: 0;
Copy link
Owner

Choose a reason for hiding this comment

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

Without the border looks a bit "random" stuff floating, we could add border-radius: 3px; to look like more the GitHub's boxes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean I removed the border because from a UX perspective giving it the "box-y" appearance makes interfaces feel more closed off, but I can understand the benefit of it since its a scrollable area and you want to communicate to the user what the container is. The bigger change was to remove the hard-coding of the background-color for the container.

@tquetano-r7
Copy link
Contributor Author

Updated per comments.

@picheli20 picheli20 merged commit b183cb9 into picheli20:master Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants