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

feat(sketch-generator): add icon merging to generator #324

Merged
merged 6 commits into from
May 21, 2021

Conversation

oddcelot
Copy link
Collaborator

@oddcelot oddcelot commented May 6, 2021

Co-authored-by: Ilmari Heikkinen [email protected]

@oddcelot oddcelot requested review from nowseemee and eeegor May 6, 2021 11:14
@oddcelot oddcelot self-assigned this May 6, 2021
@oddcelot oddcelot force-pushed the feat/sketch-icons branch 4 times, most recently from a9936cf to 6f38dbc Compare May 7, 2021 07:05
@oddcelot oddcelot changed the title WIP: feat(sketch-generator): add icon merging to generator feat(sketch-generator): add icon merging to generator May 7, 2021
@oddcelot oddcelot marked this pull request as ready for review May 7, 2021 07:07
</div>
<div class="agent-states--item">
<p class="agent-states--label">:focus</p>
<scale-link data-sketch-key="link-12" icon="{{>icon}}" href="#" data-sketch-state="focus">Link Text</scale-link>
<scale-link data-sketch-key="link-12" data-has-icon="true" href="#" data-sketch-state="focus">Link Text <scale-icon-navigation-external-link
size="16"
Copy link
Collaborator

Choose a reason for hiding this comment

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

unfortunately, prettier has no support for handlebars but you could run the html parts through to tidy up the formatting of this file - the indentation is really off here 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

did end up settings the handebars file associaction and applied html formatting

sidenote: we should move on from hbs templates, i have something leaner in mind and will explore replacement soon 🙃

Copy link
Collaborator

Choose a reason for hiding this comment

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

please test if everything still works after formatting - prettier might do weird things to the hbs code.

@nowseemee
Copy link
Collaborator

thanks for the PR Stefan, it's a lot of effort from you and is really appreciated!

it's difficult to tell what is really going on in this diff without a PR description so I left only style-related comments. I see a lot of conditions which are difficult to follow - maybe some inline code comments would help to reason about nested if blocks and else ifs?

@oddcelot oddcelot force-pushed the feat/sketch-icons branch 2 times, most recently from c3cbc8b to 3190e68 Compare May 18, 2021 09:00
@oddcelot oddcelot requested a review from nowseemee May 18, 2021 11:18
@@ -0,0 +1,8 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want this file checked in to git?

Copy link

Choose a reason for hiding this comment

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

could be
depends if we want to stay IDE agnostic^^

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

..and again, since handlebar templates will hopefully replaced, this can go

</scale-card>
<script>
/* I couldn't find a way to do this with "theming" */
document.querySelector('[data-no-padding]').styles = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a way of getting rid of styles?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, we need to take a look at the requirements for this
this is also related to #344

Copy link
Collaborator

@nowseemee nowseemee May 18, 2021

Choose a reason for hiding this comment

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

is this a blocker then?

@@ -62,3 +62,143 @@ See [Conventional Commits](https://conventionalcommits.org) for commit guideline
# [3.0.0-beta.2](https://github.com/telekom/scale/compare/v0.1.4...v3.0.0-beta.2) (2021-04-20)

**Note:** Version bump only for package @telekom/scale-generator-sketch

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you remove these additions which are before v3, please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

absolutely, wasn't sure if we wanted to keep them^^

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but yeah, nothing meaningful in there 😅

@oddcelot oddcelot force-pushed the feat/sketch-icons branch from 3190e68 to d1d95f5 Compare May 20, 2021 11:56
@oddcelot oddcelot merged commit 3b7cbd6 into main May 21, 2021
@oddcelot oddcelot deleted the feat/sketch-icons branch August 9, 2021 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants