Skip to content
This repository was archived by the owner on Aug 13, 2019. It is now read-only.

Feature/stij 129: Make inline buttons the default#111

Open
meirege wants to merge 5 commits intodevelopfrom
feature/STIJ-129
Open

Feature/stij 129: Make inline buttons the default#111
meirege wants to merge 5 commits intodevelopfrom
feature/STIJ-129

Conversation

@meirege
Copy link
Contributor

@meirege meirege commented Oct 24, 2017

No description provided.

@meirege meirege self-assigned this Oct 24, 2017
{% include '@paragraph' with { 'text': 'Normal text below an heading 6.'} %}
{% include '@paragraph' with { 'text': 'Paragraphs <a href="#">link</a>'} %}
<p>{% include '@button-sm' with { 'text': 'Button'} %}</p>
<p>{% include '@button' with { 'text': 'Button'} %}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not be better to use the same names for placeholders and the corresponding buttons in Sass

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 changed this. What do you think?

We also provide a SASS placeholder.

## Small buttons
* `%btn--small`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we implement two different button classes? This is a bit confusing. Placeholder for extends is ok.

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 think this is meant to be backwards compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jeroen005 can you check if these button classes are still necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Classy is using .button if I'm not mistaking.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. If that is the case we should leave it in for now untill we have time to refactor the basetheme to strip the classy classes and add the styleguide markup

//
@mixin btn-block {
display: block;
width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

width: 100% is not necessary, since you declare your element as a block which has a default width of 100%.

//
@mixin btn-inlineblock {
display: inline-block;
width: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

Width auto should not be necessary in this context

@meirege meirege changed the title Feature/stij 129 ON HOLD: Feature/stij 129 Oct 24, 2017
@meirege meirege added this to the Minor 2.7.0 milestone Nov 27, 2017
@meirege meirege changed the title ON HOLD: Feature/stij 129 ON HOLD: Feature/stij 129: Make inline buttons the default Nov 29, 2017
@meirege meirege modified the milestones: Minor 2.7.0, 2.8.0 Nov 29, 2017
@meirege meirege changed the title ON HOLD: Feature/stij 129: Make inline buttons the default Feature/stij 129: Make inline buttons the default Dec 8, 2017
@meirege meirege requested a review from delrueba December 8, 2017 08:37
@meirege meirege modified the milestones: 2.8.0, 3.0.0 Jan 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants