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

style: updated spot illus styles to be darkmode friendly #26

Merged
merged 5 commits into from
Nov 30, 2023

Conversation

yorbi-dp
Copy link
Contributor

Description

  • Updated old Spot Illustrations colors so they can work with Dark Mode.
  • Added 2 new Illustrations.

Disclaimer: This is a first (new) version of spot illustrations, the idea it's to improve the structure as well with more documentation and how to use on a next version.

Pull Request Checklist

  • Ask the contributors if a similar effort is already in process or has been solved.
  • Review the contribution guidelines.
  • Use staging as your pull request's base branch. (All PRs using production as its base branch will be declined).
  • Ensure all gulp scripts successfully compile.
  • Update, remove, or extend all affected documentation.
  • Ensure no private Dialpad links or info are in the code or pull request description (Dialtone is a public repo!).

Obligatory GIF (super important!)

image

@yorbi-dp yorbi-dp added documentation Improvements or additions to documentation enhancement New feature or request labels Nov 28, 2023
@yorbi-dp yorbi-dp self-assigned this Nov 28, 2023
Copy link

✔️ Dialtone Deploy Preview ready!
😎 Browse the preview: https://dialtone.dialpad.com/deploy-previews/pr-26

Copy link

✔️ Dialtone-vue 3 Deploy Preview ready!
😎 Browse the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-26

Copy link
Contributor

@francisrupert francisrupert left a comment

Choose a reason for hiding this comment

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

image

Mind and EmptyState broken in preview

@@ -54,6 +53,8 @@ import SpotMaleLaptopTyping from '@dialpad/dialtone/lib/dist/vue/spot/SpotMaleLa
import SpotFileUpload from '@dialpad/dialtone/lib/dist/vue/spot/SpotFileUpload.vue';
import SpotBrowserTableGraph from '@dialpad/dialtone/lib/dist/vue/spot/SpotBrowserTableGraph.vue';
import SpotBrowserListCallout from '@dialpad/dialtone/lib/dist/vue/spot/SpotBrowserListCallout.vue';
import SpotMind from '@dialpad/dialtone/lib/dist/vue/spot/SpotMind.vue';
import SpotEmpty from '@dialpad/dialtone/lib/dist/vue/spot/SpotEmpty.vue';
Copy link
Contributor

Choose a reason for hiding this comment

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

EmptyState is probably far too specific. Spot Illustration are mostly illustative and thus shouldn't imply too much how they'd be used — even if this one might be used for an empty state.

@yorbi-dp
Copy link
Contributor Author

image `Mind` and `EmptyState` broken in preview

Yup, looks like this might be a Cache issue. @juliodialpad it's helping me taking a look at it!

Copy link

✔️ Dialtone Deploy Preview ready!
😎 Browse the preview: https://dialtone.dialpad.com/deploy-previews/pr-26

Copy link

✔️ Dialtone-vue 3 Deploy Preview ready!
😎 Browse the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-26

@juliodialpad
Copy link
Collaborator

juliodialpad commented Nov 28, 2023

Screenshot 2023-11-28 at 5 31 13 p m

Fixed on preview after hard refreshing (cmd+shift+r) to remove browser cache.
@yorbi-dp

@yorbi-dp
Copy link
Contributor Author

Screenshot 2023-11-28 at 5 31 13 p m Fixed on preview after hard refreshing (cmd+shift+r) to remove browser cache. @yorbi-dp

Thank you!!

Copy link

✔️ Dialtone Deploy Preview ready!
😎 Browse the preview: https://dialtone.dialpad.com/deploy-previews/pr-26

Copy link

✔️ Dialtone-vue 3 Deploy Preview ready!
😎 Browse the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-26

Copy link

✔️ Deploy Preview ready!
😎 Browse the preview: https://dialpad.design/deploy-previews/pr-26
🔨 If you experience an SSL issue then wait 2 minutes and try again.

@yorbi-dp yorbi-dp force-pushed the dlt-1359-spot-illustrations branch from 06212e0 to 71fe796 Compare November 30, 2023 20:17
Copy link

✔️ Dialtone Deploy Preview ready!
😎 Browse the preview: https://dialtone.dialpad.com/deploy-previews/pr-26/

Copy link

✔️ Dialtone-vue 3 Deploy Preview ready!
😎 Browse the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-26/

@yorbi-dp yorbi-dp marked this pull request as ready for review November 30, 2023 20:23
Copy link
Contributor

@francisrupert francisrupert left a comment

Choose a reason for hiding this comment

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

@yorbi-dp, These are good to go. Thanks!

Copy link
Contributor

@braddialpad braddialpad left a comment

Choose a reason for hiding this comment

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

Looks great! mostly just changes due to new monorepo documentation.

Comment on lines 75 to 79
```yaml
{
"import SpotIlluVueName from '@dialpad/dialtone/lib/dist/vue/spot/SpotIlluVueName.vue';"
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove the braces and quotes. also set the language to js rather than yaml.

import SpotIlluVueName from '@dialpad/dialtone/lib/dist/vue/spot/SpotIlluVueName.vue';

5. Commit and push your branch to Dialtone.
6. Open a pull request.
7. Once approved it can be merged into staging and will go out in the next dialtone release.
4. Import the Illustrations into the `BaseIcon.vue` file:
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make this entire section for only spot illustrations instead of "For brand icons and spot illustrations" since brand icons have now been moved to dialtone-icons and have the same process as system icons. You should remove any steps related to brand icons from this section.

}
```

6. Verify your changes have been updated on the website by running `npm run start` and navigating to `localhost:4000`.
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of npm run start in the monorepo it should now be pnpm run start:dialtone

```

6. Verify your changes have been updated on the website by running `npm run start` and navigating to `localhost:4000`.
If you would like to verify your final output svg file run `npm run build` and look in the `./lib/dist/svg` folder
Copy link
Contributor

Choose a reason for hiding this comment

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

pnpm nx build dialtone instead of npm run build

Comment on lines 83 to 91
```yaml
{
"export default {
components: {
SpotIlluVueName,
}
}"
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

same thing here with the extra braces / quotes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Fixed all the suggestions to the contribution.md file in the lastest commit.

Copy link

✔️ Dialtone Deploy Preview ready!
😎 Browse the preview: https://dialtone.dialpad.com/deploy-previews/pr-26/

Copy link

✔️ Dialtone-vue 3 Deploy Preview ready!
😎 Browse the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-26/

Copy link
Contributor

@braddialpad braddialpad left a comment

Choose a reason for hiding this comment

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

Great thanks!

@yorbi-dp yorbi-dp merged commit 762d278 into staging Nov 30, 2023
4 checks passed
@yorbi-dp yorbi-dp deleted the dlt-1359-spot-illustrations branch November 30, 2023 22:44
Copy link

🎉 This PR is included in version 8.22.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants