Skip to content

Conversation

@fpliger
Copy link
Contributor

@fpliger fpliger commented May 23, 2022

Closes #446

@fpliger fpliger added tag: styling Related to the styling of pyscript components tag: component Related to PyScript components labels May 23, 2022
@fpliger
Copy link
Contributor Author

fpliger commented May 31, 2022

@marimeireles or anyone willing to pre-check this.... Mind a quick review and test of the examples? 🙂

@RSMuthu
Copy link

RSMuthu commented May 31, 2022

hi @fpliger, if this PR and issue is to clean tailswindcss dependency completely off, then i would suggest on removing the left outs as well. The following remnants will still be left out (as per the currently available commits):

  • the tailwindcss dependency from the package.json file
  • the complete tailswind.config.js config file
  • tailwindcss plugin setup on the rollup bundler configuration.

Copy link
Contributor

@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

It's breaking the CSS on http://localhost:8000/pyscript/examples/repl2.html.

Screenshot 2022-05-31 at 19 57 54

The output should be on the side but it's showing underneath every new cell.

Is there a reason why you didn't remove tailwind stuff? Like the ones addressed by @RSMuthu?

Other than that. It works! :)
I tested all tests.


<link rel="icon" type="image/png" href="favicon.png" />
<link rel="stylesheet" href="https://pyscript.net/alpha/pyscript.css" />
<link rel="stylesheet" href="./build/pyscript.css" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this example should point to pyscript.net, correct?

<link rel="stylesheet" href="./build/pyscript.css" />

<script defer src="https://pyscript.net/alpha/pyscript.js"></script>
<script defer src="./build/pyscript.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same ^

<link rel="icon" type="image/png" href="favicon.png" />
<link rel="stylesheet" href="https://pyscript.net/alpha/pyscript.css" />
<script defer src="https://pyscript.net/alpha/pyscript.js"></script>
<link rel="stylesheet" href="./build/pyscript_base.css" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also point to pyscript.net?
And I don't think we have the pyscript_base file, at least my npm build is not generating this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... I messed up editing the wrong files while it was in progress.... Fixed

<link rel="stylesheet" href="https://pyscript.net/alpha/pyscript.css" />
<script defer src="https://pyscript.net/alpha/pyscript.js"></script>
<link rel="stylesheet" href="./build/pyscript_base.css" />
<script defer src="./build/pyscript.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Comment on lines 10 to 13
<link rel="stylesheet" href="./build/pyscript_base.css" />
<link rel="stylesheet" href="repl.css" />

<script defer src="https://pyscript.net/alpha/pyscript.js"></script>
<script defer src="./build/pyscript.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here as above comments

Comment on lines 2 to 4
/* @tailwind base; */
/* @tailwind components; */
/* @tailwind utilities; */
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove all of these comments?

@fpliger fpliger marked this pull request as ready for review June 13, 2022 22:20
@marimeireles
Copy link
Contributor

@fpliger you need more specific review on this?
I can give a try on my rusty CSS for us to merge this :)

@fpliger
Copy link
Contributor Author

fpliger commented Jun 21, 2022

@marimeireles no... mainly to check if it looks good to you and have more eyes checking the examples and other places I may have broken the code

@fpliger
Copy link
Contributor Author

fpliger commented Jun 21, 2022

Thanks @RSMuthu that's a great point :)

@fpliger
Copy link
Contributor Author

fpliger commented Jun 21, 2022

Also worth mentioning that since we are planning a release (the first release after we adopt the new versioning strategy) I'd like to merge this after we release.

@fpliger
Copy link
Contributor Author

fpliger commented Jun 21, 2022

Should have addressed comments from both @marimeireles and @RSMuthu . Ready for another round :)

Copy link
Contributor

@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

@fpliger this is so much CSS!
Thank you for the work here =)
It's a 👍 from me, other than these minor nitpicks.

this.widths.forEach((width, index) => {
const node: ChildNode = mainDiv.childNodes[index];
addClasses(node as HTMLElement, [width, 'mx-1']);
// addClasses(node as HTMLElement, [width, 'mx-1']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this?

</div>
</div>`;
</div>
</div>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a ;?

@fpliger
Copy link
Contributor Author

fpliger commented Jun 24, 2022

great catches, thank you @marimeireles . Should be good now

@fpliger fpliger merged commit fcaa573 into main Jun 24, 2022
@fpliger fpliger deleted the fpliger/447_remove_tailwind branch June 24, 2022 23:30
@fpliger fpliger mentioned this pull request Jul 20, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tag: component Related to PyScript components tag: styling Related to the styling of pyscript components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace Tailwind with Pure CSS

4 participants