Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

Fix InterfaceOverview to properly close <dl>s #1073

Closed

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Feb 7, 2019

* Also removed the Experimental badge and merged the
Obsolete badge with the Deprecated one.

* Put back in the badges I removed in the original PR;
I'll submit a separate PR for that soon.

* Don't show the Deprecated badge if the Obsolete tag is
present (unless Deprecated is as well).

* Fix incorrect indentation
@ExE-Boss ExE-Boss closed this Feb 7, 2019
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 7, 2019

Travis CI is once again not running the build correctly. 🤦🏻‍♂️

Copy link
Contributor

@davidflanagan davidflanagan left a comment

Choose a reason for hiding this comment

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

Thanks for this @ExE-Boss. I hate to say this, but could you submit a different version where you don't run Prettier on the macro? The formatting changes make it harder to figure out what the bug fix is.

Also, I don't see anything in this PR or the one it supersedes that explains what the bug is that is being fixed. I need a bit of context so I understand what is being fixed before I review the fix.

I have reviewed your test file and have left some comments on that. I've got some style nits there, and some suggestions for code that could usefully be moved into utils.js for use by other tests.

macros/InterfaceOverview.ejs Outdated Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Outdated Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Outdated Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Outdated Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Outdated Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Outdated Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Outdated Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Outdated Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Show resolved Hide resolved
@ExE-Boss ExE-Boss force-pushed the macros/interface-overview/fix-dl-imbalance branch from e74818f to 6a2f824 Compare February 7, 2019 22:06
@ExE-Boss ExE-Boss closed this Feb 7, 2019
@ExE-Boss ExE-Boss reopened this Feb 7, 2019
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 7, 2019

Looks like Travis broke, again.

Copy link
Contributor

@davidflanagan davidflanagan left a comment

Choose a reason for hiding this comment

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

@ExE-Boss: I'd like another round of changes. In particular I don't know what the right thing to do about the groupdata changes. I think we'd need Chris or @a2sheppy to weigh in on that, so it might be simpler to just not change the group stuff at all in this PR.

I've got a few suggestions about the changes to utils.js, but I think they're going to be really useful for other tests. @escattone and @wbamberg: I've suggested that ExE-Boss add readJSONFixture() and macro.mockTemplate() to the test utilities. If you have a chance, please take a look at those new functions and how they're used here in the InterfaceOverview.test.js file.

macros/InterfaceOverview.ejs Outdated Show resolved Hide resolved
macros/InterfaceOverview.ejs Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Outdated Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Show resolved Hide resolved
tests/macros/InterfaceOverview.test.js Outdated Show resolved Hide resolved
tests/macros/utils.js Show resolved Hide resolved
tests/macros/utils.js Show resolved Hide resolved
tests/macros/utils.js Show resolved Hide resolved
tests/macros/utils.test.js Show resolved Hide resolved
tests/macros/utils.js Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss force-pushed the macros/interface-overview/fix-dl-imbalance branch 2 times, most recently from fc9ed77 to 5a046e2 Compare February 21, 2019 16:58
@ExE-Boss
Copy link
Contributor Author

review?(@a2sheppy, @davidflanagan)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants