No longer adds start attribute to unordered lists #1901
Conversation
There was a problem hiding this comment.
It looks like this was branched from a pretty old commit - the most recent tag in this branch is v11.2.0, dev/16.
All pull requests should also target the latest dev branch - at the moment, that is dev/25-bixbite.
Target the latest dev branch (dev/25-bixbite), then either merge that branch into this one or make a new branch from dev/25 and rewrite these changes on that new branch.
FrenjaminBanklin
changed the base branch from
dev/25-bixbite
to
dev/26-neon-apatite
FrenjaminBanklin
changed the base branch from
dev/26-neon-apatite
to
dev/28-jadeite
There was a problem hiding this comment.
There are snapshot failures in this obonode's viewer component tests that need to be resolved - depending on the test this may mean simply updating the snapshot, but if the snapshot should still be correct then make sure there aren't new bugs.
You should also make sure the tests are thorough with regards to the new functionality - make sure that the start attribute is present for ordered lists and absent for unordered lists.
| @@ -164,6 +156,7 @@ describe('List Styles', () => { | |||
| const ls = new ListStyles('ordered') | |||
|
|
|||
| expect(ls.get(0).toDescriptor()).toEqual({ | |||
| // | |||
There was a problem hiding this comment.
| // |
FrenjaminBanklin
changed the base branch from
dev/28-jadeite
to
dev/29-sodalite
|
I removed the extra comments and cleaned up the code. I also fixed the snapshot failures by running yarn test -u. I have not yet verified that this all functions and fulfills the task yet. |
There was a problem hiding this comment.
In general I wouldn't suggest using the -u flag since it'll just update all snapshots. I tend to go through failing snapshots one at a time and make sure that the changes to the snapshot are what I would expect them to be based on the changes to the code.
That said, these snapshot changes seem to be fine. Not sure why a caliper-related snapshot was still hanging around or what this PR has to do with it, but I'll let that one go since it shouldn't have still been there anyway.
I think it would also be prudent to explicitly test that unordered lists don't have a start attribute and that ordered lists do have a start attribute.
|
"I think it would also be prudent to explicitly test that unordered lists don't have a start attribute and that ordered lists do have a start attribute." To do this, would I add this expectation to the list-styles.test.js file that is already edited on this PR ? |
|
I would probably make a new |
RachelDau
force-pushed
the
issue/1279-no-start-element
branch
4 times, most recently
from
30c8759 to
f91fa42
Compare
RachelDau
force-pushed
the
issue/1279-no-start-element
branch
from
f91fa42 to
999f245
Compare
|
I wrote two unit tests. One that checks that ordered lists have a start attribute ( I check the existence is 'true'), and the other that checks that unordered lists do not have a start attribute (I check that the existence is 'undefined'). Please review that these accomplish the intent and are a good way to verify. |
| test('all unordered lists dont have a start attribute', () => { | ||
| const ls = new ListStyles() | ||
| ls.init() | ||
| if (ls.type === 'unordered') { |
There was a problem hiding this comment.
Given the expectation for ls.type to be one or the other, these conditions are unnecessary. In a worst-case scenario, not that I know how this might happen, this conditional could allow the test to pass even if the expectations are not met, since the expectations might not be checked at all.
Also this is just nitpicking, but if unordered is the default behavior, I would keep a test where it is not set (to make sure it's the default behavior) and also a separate test explicitly setting 'unordered' to make sure everything still reacts properly.
There was a problem hiding this comment.
I made these changes and took a closer look at the unit test for ordered which was filing with the removal of the condition. I learned ls.init() sets the style to unordered so I removed this line. I added ls.get(indent) as it calls other functions that seem to set up the list including adding the start attribute if it is ordered. I think this function would be called to create a list with defaults so I think it is a better way to test that these three unit tests function properly.
A start attribute is not automatically added when const ls = new ListStyles('ordered') is created so this test was failing before I called ls.get(indent) which returns getStyleWithDefaults(indent, this.type, this.styles[indent]) as seen in list-styles.js
No description provided.