Skip to content

feat: adds gap property #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: adds gap property #29

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39a94d9
feat: adds gap property
demetrius-mp May 1, 2022
01fec8b
feat: adds test for bad value on gap property
demetrius-mp May 1, 2022
6a820bd
Merge branch 'himynameisdave:main' into feat/gap-property
demetrius-mp Sep 16, 2022
840eac4
fix: gap prop defaults to undefined, and remove test for unset gap prop
demetrius-mp Sep 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Flex.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export interface FlexProps {
justify?: Justify;
direction?: Direction;
reverse?: boolean;
gap?: string;
}

export default class Flex extends SvelteComponentTyped<FlexProps> {}
3 changes: 3 additions & 0 deletions src/Flex.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
export let align = 'center';
export let justify = 'center';
export let reverse = false;
export let gap = '0';
Copy link
Owner

Choose a reason for hiding this comment

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

This should be a number, not a string.

Suggested change
export let gap = '0';
export let gap = 0;

Eventually I'd like to add Typescript definitions for this package, which would help make this more clear.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I think this should be the initial value for gap, which I think is 'normal normal' (based on MDN). Either that or we should perhaps leave it undefined/null or something, as I don't want it to be applied if consumers aren't using it.

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 that leaving as null / undefined would be better (because it wouldnt be applied)

Copy link
Owner

Choose a reason for hiding this comment

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

Yes let's leave it as null or undefined


// 'start' | 'center' | 'end' | 'stretch'
const alignMap = {
Expand All @@ -22,6 +23,7 @@
evenly: 'space-evenly',
};


$: directionWithReverse = reverse ? `${direction}-reverse` : direction;
</script>

Expand All @@ -32,6 +34,7 @@
style:flex-direction={directionWithReverse}
style:align-items={alignMap[align]}
style:justify-content={justifyMap[justify]}
style:gap={gap}
>
<slot />
</div>
20 changes: 20 additions & 0 deletions src/__tests__/Flex.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,4 +140,24 @@ describe('<Flex /> component', () => {
expect(container).not.toHaveStyle('flex-direction: column'); // default/fallback
});
});

describe('gap prop', () => {
test('has gap prop set to 1 rem', () => {
const container = renderFlex({ gap: '1rem' });
expect(container).toHaveStyle('gap: 1rem');
expect(container).not.toHaveStyle('gap: 0rem');
});

test('does not has gap prop', () => {
const container = renderFlex();
expect(container).toHaveStyle('gap: 0');
expect(container).not.toHaveStyle('gap: 1rem');
});

test('bad value', () => {
const container = renderFlex({ gap: 'oops' });
expect(container).not.toHaveStyle('gap: 1rem');
expect(container).not.toHaveStyle('gap: 0');
Copy link
Owner

Choose a reason for hiding this comment

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

Great tests! 😄

Perhaps we can also add an expected fallback/default value? (the initial value we discussed above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how can i test this? as far as i know, if i make the default value for the gap prop equal to undefined, the svelte style directive would just ignore it and not pass anything.
should i expect(container).not.toHaveStyle('gap: 0')?

Copy link
Owner

@himynameisdave himynameisdave Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Because the initial value for gap is 'normal normal', I think it would be a great idea to do a positive check for that value (I think that testing-library's toHaveStyle should return true because it is using getComputedStyle under the hood, which always returns whatever the initial value is if none is set.

Try this:

expect(container).toHaveStyle({ gap: 'normal normal' });

If not, I'd say the tests you've already added are sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried expecting it to have gap: 'normal normal' but it didnt work. im going to remove the test for "does not has gap prop", but feel free to add it after merging the PR. also, pls check my comment on #30

});
})
});