Skip to content
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.

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

fix: Module reference issue with or-type variant #1606

Merged
merged 10 commits into from
Jan 3, 2024
Merged

Conversation

harshdoesdev
Copy link
Contributor

@harshdoesdev harshdoesdev commented Dec 14, 2023

It fixes an issue where property values were not being wrapped in their respective or-type when a reference value was assigned to a property by referencing it from a header module. In the below example, the $page.colors.base- was not being wrapped in the or-type fastn_dom.BackgroundStyle.Solid, but $colors.base- was properly being wrapped in the or-type. Due to this the background color for the first column was not changing where the value was not wrapped in its respective or-type.

-- import: 08-inherited as colors

-- component page:
module colors: colors

-- ftd.column:
width: fill-container
height: fill-container
background.solid: $page.colors.base-

-- ftd.text: Hello world

-- ftd.column:
width: fill-container
height: fill-container
background.solid: $colors.base-

-- ftd.text: Hello world

-- end: ftd.column

-- end: ftd.column

-- end: page

-- page:

@amitu
Copy link
Contributor

amitu commented Dec 20, 2023

@Arpita-Jaiswal, what to do with this PR?

@amitu
Copy link
Contributor

amitu commented Dec 21, 2023

@harshdoesdev what do we do with this PR? Is it still needed? Ready?

@harshdoesdev
Copy link
Contributor Author

harshdoesdev commented Dec 21, 2023

@harshdoesdev what do we do with this PR? Is it still needed? Ready?

Yes @amitu, this PR is complete. It fixes an issue where property values were not being wrapped in their respective or-type when a reference value was assigned to a property by referencing it from a header module. In the below example, the $page.colors.base- was not being wrapped in the or-type fastn_dom.BackgroundStyle.Solid but $colors.base- was properly being wrapped in the or-type. Due to this the background color for the first column was not changing where the value was not wrapped in its respective or-type.

-- import: 08-inherited as colors

-- component page:
module colors: colors

-- ftd.column:
width: fill-container
height: fill-container
background.solid: $page.colors.base-

-- ftd.text: Hello world

-- ftd.column:
width: fill-container
height: fill-container
background.solid: $colors.base-

-- ftd.text: Hello world

-- end: ftd.column

-- end: ftd.column

-- end: page

-- page:

@amitu
Copy link
Contributor

amitu commented Dec 21, 2023

@harshdoesdev I was expecting something like this:

what do we do with this PR?

Merge this PR.

Is it still needed?

Yes

Ready?

Yes, this PR is ready from my side.

You have used a lot of words and managed to not answer any of my questions :-p

@harshdoesdev
Copy link
Contributor Author

It fixes an issue where property values were

Yes, @amitu, this pull request is complete from my side. It is still needed, and we can merge this pull request.

@amitu
Copy link
Contributor

amitu commented Dec 21, 2023

@Arpita-Jaiswal, @Heulitig, review Ker lo

@amitu
Copy link
Contributor

amitu commented Dec 29, 2023

@harshdoesdev there is no test case using or-type in this PR, can you add one?

@harshdoesdev
Copy link
Contributor Author

harshdoesdev commented Dec 29, 2023

@harshdoesdev there is no test case using or-type in this PR, can you add one?

@amitu I have added an or-type in the test

@amitu
Copy link
Contributor

amitu commented Dec 29, 2023

Did you miss checking in 85-export-or-type.ftd?

@harshdoesdev
Copy link
Contributor Author

Did you miss checking in 85-export-or-type.ftd?

No, it is an existing test file

amitu
amitu previously approved these changes Dec 29, 2023
@amitu
Copy link
Contributor

amitu commented Dec 29, 2023

@Arpita-Jaiswal, merge it?

@Heulitig Heulitig merged commit daa2641 into main Jan 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants