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

Consider table header content when double-clicking to resize column #61

Merged
merged 2 commits into from
Nov 11, 2016

Conversation

cmslewis
Copy link
Contributor

@cmslewis cmslewis commented Nov 10, 2016

Before:
Only body content is considered when auto-resizing a column on double-click.
before

After:
(No header content)
after3

(With header content, which we ignore when recalculating column width)
after
after2

@giladgray
Copy link
Contributor

hmm your last gif of the "too wide" also puts all the text on one line. i wonder if that's why it gets so large? i don't really know how the table is used but if that sort of use case is at all common then it would be a blocking issue.

const cellClasses = [
`.bp-table-cell-col-${columnIndex}`,
".bp-table-column-name",
".bp-table-header-content",
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we should not consider this element? that would solve the issue in your 3rd gif, wouldn't it?

it makes intuitive sense to consider cell content and column title. i think we can safely ignore header content here because users can put whatever they want there (as demonstrated by this example) and it could get weird fast.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I could kind of go either way. But I think Gilad's idea of ignore header content probably makes the most sense to the uses we see in the field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@cmslewis
Copy link
Contributor Author

Sorry, missed the notifications here (still getting all of that set up). Will amend.

@cmslewis
Copy link
Contributor Author

@giladgray ready for re-review

@blueprint-bot
Copy link

Ignore header content

Preview: docs | landing | table
Coverage: core | datetime

@giladgray giladgray merged commit 2a4ab73 into master Nov 11, 2016
@giladgray giladgray deleted the cl/double-click-resize-consider-headers branch November 11, 2016 00:46
@giladgray giladgray mentioned this pull request Nov 11, 2016
@cmslewis cmslewis restored the cl/double-click-resize-consider-headers branch November 11, 2016 23:11
@adidahiya adidahiya deleted the cl/double-click-resize-consider-headers branch November 12, 2016 15:38
@cmslewis cmslewis restored the cl/double-click-resize-consider-headers branch November 14, 2016 23:45
@adidahiya adidahiya deleted the cl/double-click-resize-consider-headers branch November 15, 2016 02:52
greglo pushed a commit to greglo/blueprint that referenced this pull request Dec 12, 2016
peterblazejewicz pushed a commit to peterblazejewicz/blueprint that referenced this pull request Dec 20, 2017
Add component jsdoc description to sidebar
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants