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

Use mdjs-layout #4

Merged
merged 7 commits into from
Feb 3, 2022
Merged

Use mdjs-layout #4

merged 7 commits into from
Feb 3, 2022

Conversation

bashmish
Copy link
Contributor

@bashmish bashmish commented Jan 5, 2022

Integrated mdjs-layout with all goodies from light/dark modes to good typography, nicely decorated demos and so on.

@bashmish
Copy link
Contributor Author

bashmish commented Jan 18, 2022

@m4dz this is almost ready! I think dockit-core needs some polishing, because on the starter kit side it's all correct now, but if we merge this now, a few tokens will be shown incorrectly. Actually the current master also contains a few bugs due to dockit-core, which I also fixed, but new ones became visible due to Prism and other styles of the mdjs-layout, so dockit-core needs even more improvements. Hopefully it's not a super prio, because I need to switch to another task now, but I'm planning to return to this ASAP.

@bashmish bashmish force-pushed the feat/use-mdjs-layout branch 2 times, most recently from 285032a to 1e0acf2 Compare January 31, 2022 19:55
@bashmish bashmish marked this pull request as ready for review January 31, 2022 19:57
@bashmish
Copy link
Contributor Author

@m4dz finally finished this :) I managed to fix a lot of small things, which are present in the current main branch anyway and not related to mdjs-layout migration, but just became visible thanks to that, but not everything is clear how to fix. I'll leave it up to you to decide as you probably know better. Read my comments about particular code.

<template>
<div>
<Button outlined name="Rioters 🤘" />
<MyButton outlined name="Rioters 🤘" />
Copy link
Contributor Author

@bashmish bashmish Jan 31, 2022

Choose a reason for hiding this comment

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

I had to change vue lang to html to make sure the code highlighting is working. But this activates prettier which doesn't like capital letters in native tags, which was introduced to rewrite e.g. <BUTTON> to <button>, but the same happens to <Button> -> <button> which is undesired. So I renamed the tag to <MyButton> to workaround this behavior. Maybe best to introduce a prefix for all components in the DS so that all component names do not overlap with native HTML tags? Up to you what's the best fix here.

css-props-prefix="--color"
component-class="box"
style-key="background-color"
></dockit-sass-showcases>
></dockit-css-showcases>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since I fixed dockit-css-showcases to properly show opacity in the documentation context, it became clear that one of the colors you have is transparent... not sure if you meant that or created such color by mistake, please double check

image

css-props-prefix="--shadow"
component-class="box"
style-key="box-shadow"
checkered-background="false"
></dockit-sass-showcases>
></dockit-css-showcases>
Copy link
Contributor Author

@bashmish bashmish Jan 31, 2022

Choose a reason for hiding this comment

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

elevation is not visible in the dark mode

one might think it's because of mdjs-layout typography which has too dark background which is not compatible with the DS (and doesn't set var(--doc-color-background) on the body) , but I think the shadows themselves are not dark compatible, so I'd recommend to consider adding 2 sets of shadows for light and dark separately

if you feel like it's because of mdjs-layout or how I set it up, we should discuss it
I have a well-thought vision here and prefer to have it discussed thoroughly before changing to smth different, the devil is in the details here

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I agree on having two different set of shadows for light/dark modes. Thanks 👍

<dockit-sass-showcases
```html story
<dockit-css-showcases
mode="time"
Copy link
Contributor Author

@bashmish bashmish Jan 31, 2022

Choose a reason for hiding this comment

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

I had to add mode="time" here, because --transition has been deprecated in favor of --time in dockit-core

just pay attention to this and maybe consider actually changing the prefix to --time
I just didn't feel like such renaming belongs to this PR, so left it untouched

@bashmish bashmish requested a review from m4dz February 2, 2022 11:04
@bashmish
Copy link
Contributor Author

bashmish commented Feb 2, 2022

Beeing a bot here :D (in next PRs it should be done automatically)

👀 Review in Backlight.dev

💻 Link the PR in your app (?)

npx backlight@latest link @divriots/starter-vue3 --branch pr:4@bashmish

@bashmish bashmish mentioned this pull request Feb 2, 2022
@bashmish bashmish force-pushed the feat/use-mdjs-layout branch from 1e0acf2 to 79ce950 Compare February 2, 2022 12:37
Copy link
Contributor

@m4dz m4dz left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks a lot for the good work, I'm gonna push it forward now 🙏

@bashmish bashmish merged commit 9dc1f71 into main Feb 3, 2022
@bashmish bashmish deleted the feat/use-mdjs-layout branch February 3, 2022 20:45
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