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

Add new tab button #55

Open
wants to merge 12 commits into
base: gh-pages
Choose a base branch
from
Open

Conversation

power-f-GOD
Copy link

Hi, Adam. Kindly review and accept my pull request if you will. Thanks.

@adamschwartz adamschwartz self-requested a review October 7, 2019 20:12
Copy link
Owner

@adamschwartz adamschwartz left a comment

Choose a reason for hiding this comment

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

Hey there @power-f-GOD. Thanks so much for submitting this PR. Support for a new tab button is definitely something this project is sorely lacking. I haven’t had a chance to review your PR fully. But there are a few (mostly stylistic) things I that jumped out at me right away that I’d want to take care of before diving deeper. Thanks

.gitignore Outdated
@@ -1,3 +1,4 @@
.DS_Store
bower_components/
node_modules/
.jshintrc
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this line.

css/chrome-tabs-dark-theme.styl Outdated Show resolved Hide resolved
Comment on lines 21 to 43
.new-tab-button-wrapper
display: inline-flex
align-items: center
justify-content: center
margin-left: 0

.new-tab-button
height: 30px
width: 30px
line-height: 0
border-radius: 50%
font-weight: 100
font-size: 16px
padding: 0
border: none
background: none
color: #555
box-shadow: none
transition: background 0.35s
cursor default

&:hover
background: rgba(150, 150, 150, 0.25)
Copy link
Owner

Choose a reason for hiding this comment

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

Remove the colons.

index.html Outdated
Comment on lines 115 to 116
<!-- <script src="https://unpkg.com/draggabilly@2.2.0/dist/draggabilly.pkgd.min.js"></script> -->
<script src="js/draggabilly.min.js"></script>
Copy link
Owner

Choose a reason for hiding this comment

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

While it may make sense to bring in Draggabilly directly as you have done here, if we decided to do so I’d want to separate that out from this PR so it’s only concern is the new tab button (and I’d also want to remove the commented line).

@power-f-GOD
Copy link
Author

Hi, Adam. Thanks for your response. I have made the changes you requested. Kindly review. Thanks.

@adamschwartz
Copy link
Owner

Hey @power-f-GOD. Thanks again for working on this. Another issue I’m seeing is that chrome-tabs.js appears to have been linted or formatted in some other way. Please ensure all of the changes in this PR are directly related to the addition of the new tab button and its functionality. Only then will I be able to give a complete review. Thanks

@power-f-GOD
Copy link
Author

Hi, Adams. I've made the change you requested. Kindly review. Thanks.

@power-f-GOD
Copy link
Author

Hiyi.

@xenkuo
Copy link

xenkuo commented Nov 17, 2020

Any update about this feature? It's really a necessary function, people including me are waiting for this for months.

@power-f-GOD
Copy link
Author

Oh. This PR is still open (three years later). 🥲

const TAB_CONTENT_OVERLAP_DISTANCE = 1

const TAB_OVERLAP_DISTANCE = (TAB_CONTENT_MARGIN * 2) + TAB_CONTENT_OVERLAP_DISTANCE
const TAB_OVERLAP_DISTANCE =
TAB_CONTENT_MARGIN * 2 + TAB_CONTENT_OVERLAP_DISTANCE
Copy link
Owner

Choose a reason for hiding this comment

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

Why the change in formatting here?

@@ -7,17 +7,19 @@
window.ChromeTabs = factory(window, window.Draggabilly)
}
})(window, (window, Draggabilly) => {
const TAB_CONTENT_MARGIN = 9
const TAB_CONTENT_MARGIN = 10
Copy link
Owner

Choose a reason for hiding this comment

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

Why the change from 9 to 10 here?


const TAB_CONTENT_MIN_WIDTH = 24
const TAB_CONTENT_MAX_WIDTH = 240

const TAB_SIZE_SMALL = 84
const TAB_SIZE_SMALLER = 60
const TAB_SIZE_MINI = 48
const NEW_TAB_BUTTON_AREA = 90
Copy link
Owner

Choose a reason for hiding this comment

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

NEW_TAB_BUTTON_AREANEW_TAB_BUTTON_WIDTH ?

@@ -50,6 +52,12 @@
</div>
`

const newTabButtonTemplate = `
<div class="new-tab-button-wrapper">
<button class="new-tab-button">✚</button>
Copy link
Owner

Choose a reason for hiding this comment

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

Need an <svg/> icon here to render this consistently across devices.

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.

4 participants