Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Conversation

topherfangio
Copy link
Contributor

@topherfangio topherfangio commented Aug 4, 2016

Currently, the title text of a toolbar wraps around on small screens causing the toolbar to become larger than expected.

  • Provide a new md-truncate component that applies the appropriate styles
    for using CSS-based text-overflow with an ellipsis.

    This component is designed to work nicely with the Toolbar component,
    but is also reusable and can be used in any other component.

  • Add docs/demos for the new component.

  • Update Toolbar demos to use new md-truncate component and show proper
    usage.

Additionally, add the appropriate CSS docs in a new CSS & Styles section which includes a new doc-only directive (docs-css-api-table) that creates a clean way to view documentation regarding available CSS classes.

Fixes #9026.


Edit: Add screenshot of new docs section.

screen shot 2016-08-03 at 6 56 33 pm

@topherfangio topherfangio added the needs: review This PR is waiting on review from the team label Aug 4, 2016
@topherfangio
Copy link
Contributor Author

@ThomasBurleson @EladBezalel @crisbeto @devversion Can you guys review and see if you like the color/styling of the new "CSS & Styles" section of the Toolbar docs?

Also, standard review of my proposed solution to the actual problem 😄

@ThomasBurleson
Copy link
Contributor

@topherfangio - this looks very cool. Like your use of the Component API in the docs-css-api-table implementation.

@ThomasBurleson
Copy link
Contributor

@topherfangio - lgtm

@ThomasBurleson ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Aug 26, 2016
}
}

function DocsCssApiTableController() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Why are there controllers if they aren't used? Same for the selector directive.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you simply use :

controller : function() {  },

@devversion
Copy link
Member

LGTM aside from that minor comment 👍

@topherfangio
Copy link
Contributor Author

@ThomasBurleson @devversion Waiting for Travis to succeed, but I pushed the requested updates, and fixed a typo 😃

flex: 1;
width: 0;

span {
Copy link
Member

Choose a reason for hiding this comment

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

Why not use &:first-child and make it more generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the only formatting this applies is the overflow, I wanted something that shouldn't apply any additional formatting.

I guess I'm afraid that if we switch it to :first-child, people might try to use a <div> or something else strange and it will mess up.

I'm not opposed to switching it, and I honestly can't put a finger on why I feel this way, but using the <span> just feels like a better solution to me 😕

Thoughts?

@ThomasBurleson
Copy link
Contributor

@topherfangio - can you fix the failing tests ?

@topherfangio
Copy link
Contributor Author

@ThomasBurleson Um...I "fixed" the tests...actually, I didn't do anything. Someone must have rerun the failed build and it passed?

I ran the tests on my Windows box twice. The first time a $mdBottomsheet test failed; the second time, they all passed. So I think these are erroneous failures.

@topherfangio topherfangio force-pushed the fix-toolbar-text-overflow-9026 branch from 77612f0 to a95ea71 Compare August 29, 2016 22:56
@topherfangio topherfangio added needs: work and removed pr: merge ready This PR is ready for a caretaker to review labels Aug 29, 2016
@topherfangio
Copy link
Contributor Author

Talked with Elad, we think making this an actual directive of <md-toolbar-title> makes more sense instead of forcing people to include a class and then add a <span> tag inside.

@ThomasBurleson Unless you disagree, I'll get this done tomorrow.

@topherfangio topherfangio force-pushed the fix-toolbar-text-overflow-9026 branch from a95ea71 to f7d4cb4 Compare September 1, 2016 21:43
@topherfangio topherfangio added needs: review This PR is waiting on review from the team and removed needs: work labels Sep 1, 2016
@topherfangio
Copy link
Contributor Author

topherfangio commented Sep 1, 2016

@ThomasBurleson @EladBezalel Made a more generic <md-title> component which handles this case for any component (not just Toolbar).

Do you like the name md-title, or should we perhaps name it md-ellipsis to be more clear?

@topherfangio topherfangio added this to the 1.1.2 milestone Sep 1, 2016
@topherfangio topherfangio removed this from the 1.1.1 milestone Sep 1, 2016
@EladBezalel
Copy link
Member

EladBezalel commented Sep 1, 2016

I said that maybe having an ellipsis directive would be nice but in case we don't want to create more complexibility than I'd go with <md-toolbar-title>

@ThomasBurleson
Copy link
Contributor

md-truncate

@ThomasBurleson ThomasBurleson added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: work and removed needs: review This PR is waiting on review from the team labels Sep 7, 2016
Currently, the title text of a toolbar wraps around on small screens
causing the toolbar to become larger than expected.

- Provide a new `md-truncate` component that applies the appropriate
  styles for using CSS-based text-overflow with an ellipsis.

  This component is designed to work nicely with the Toolbar component,
  but is also reusable and can be used in any other component.

- Add docs/demos for the new component.

- Update Toolbar demos to use new `md-truncate` component and show proper
  usage.

Additionally, add the appropriate CSS docs in a new CSS & Styles section
which includes a new doc-only directive (`docs-css-api-table`) that
creates a clean way to view documentation regarding available CSS classes.

Fixes angular#9026.
@topherfangio topherfangio force-pushed the fix-toolbar-text-overflow-9026 branch from f7d4cb4 to 2c0555d Compare October 20, 2016 22:34
@topherfangio topherfangio added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: work labels Oct 20, 2016
@topherfangio
Copy link
Contributor Author

Updated to md-truncate. Ping @ThomasBurleson for one more review.

@ThomasBurleson ThomasBurleson added needs: presubmit and removed needs: review This PR is waiting on review from the team labels Oct 20, 2016
@ThomasBurleson
Copy link
Contributor

@topherfangio - lgtm

@kara kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Nov 16, 2016
@kara kara merged commit 284d422 into angular:master Nov 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants