Skip to content

Append extra classes to existing ones in the body and html tags#268

Merged
kratiahuja merged 5 commits intoember-fastboot:masterfrom
cibernox:fix-double-class-bug
Apr 7, 2020
Merged

Append extra classes to existing ones in the body and html tags#268
kratiahuja merged 5 commits intoember-fastboot:masterfrom
cibernox:fix-double-class-bug

Conversation

@cibernox
Copy link
Contributor

@cibernox cibernox commented Mar 23, 2020

Before this change, if users added classes dynamically to the body or html tags, and those tags already had any class defined, the resulting HTML would be:

<html class="default-class" class="dynamic-class">

which is obviously invalid. Classes are afaik the only html attribute that we want to concatenate instead of just blindly appending, so I added special treatment for the class attribute only.

@cibernox cibernox force-pushed the fix-double-class-bug branch from 7ef8733 to c1454ef Compare March 23, 2020 21:20
@cibernox
Copy link
Contributor Author

@snewcomer this should fix our bug

Copy link
Contributor

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

Wow! This was a really nice implementation. Noticed a rogue file here fyi...

Screen Shot 2020-03-24 at 9 45 10 PM

Copy link

@cbroeren cbroeren left a comment

Choose a reason for hiding this comment

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

Great work!

});
});

it('appends classes correctly even when there was no classes in the original html', function() {

Choose a reason for hiding this comment

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

Is this something you want to test for the html classes as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added another test. Good point.

@snewcomer
Copy link
Contributor

snewcomer commented Mar 25, 2020

ref #171 Notifying @ef4 if he has any interest in this PR!

What this fixes - this is what the html looks like sent down the pipe with an existing class in index.html

<body class="original" data-foo class="new">

will now be

<body class="original new" data-foo>

@rwjblue rwjblue requested a review from kratiahuja March 26, 2020 12:30
@kratiahuja
Copy link
Contributor

Lgtm!

@cibernox
Copy link
Contributor Author

cibernox commented Apr 7, 2020

Great, if there is anything else I must address to merge this please let me know.

@kratiahuja
Copy link
Contributor

Sorry forgot to click merge last night.

@kratiahuja kratiahuja merged commit b450f7d into ember-fastboot:master Apr 7, 2020
@kratiahuja
Copy link
Contributor

fastboot@3.0.3 has been published

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