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: Adds new tokens to Skeleton #171

Merged
merged 25 commits into from
Jul 25, 2022

Conversation

lucasfp13
Copy link
Contributor

@lucasfp13 lucasfp13 commented Jul 12, 2022

What's the purpose of this pull request?

This PR intends to apply new tokens to Skeleton and its related components, based on the new Theming structure: new scoped tokens linked to global ones.

It also uses FastStore UI's Skeleton component and renames SkeletonElement to Skeleton for a better code understanding/legibility.

How does it work?

This PR uses local variables to parameterize the Skeleton and its related components' properties, then connects these scoped tokens to the global ones.

How to test it?

These changes should not affect the skeletons' behavior. It can be tested through browser developer tools, applying a network throttling to simulate, for instance, a slow 3g connection.

skeleton.mov

Checklist

Changelog

  • Added an entry in the CHANGELOG.md at the beginning of its due section. The latest version should comes first.
  • Added the PR number with the PR link at the entry in the CHANGELOG.md. E.g., New items in the pull_request_template.md (#4)

PR Description

  • Added a label according to the PR goal - Breaking change, Enhancement, Bug or Chore.
  • Added the component, hook, or pathname in-between backticks (``) - If applicable. E.g., ComponentName component.
  • Identified the function or parameter in the PR - If applicable. E.g., useWindowDimensions hook.

Documentation

  • PR description

@lucasfp13 lucasfp13 self-assigned this Jul 12, 2022
@vercel
Copy link

vercel bot commented Jul 12, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nextjs-store-storybook ✅ Ready (Inspect) Visit Preview Jul 25, 2022 at 1:09PM (UTC)

@lucasfp13 lucasfp13 added Chore General tasks. Features New feature or request and removed Chore General tasks. labels Jul 15, 2022
@lucasfp13 lucasfp13 force-pushed the feat/FS-269-add-new-tokens-to-skeleton branch from 6e2d583 to 4002844 Compare July 15, 2022 21:40
@vercel vercel bot temporarily deployed to Preview July 15, 2022 21:41 Inactive
@vtex-sites
Copy link

vtex-sites bot commented Jul 15, 2022

Preview is ready

This pull request generated a Preview

👀   Preview: https://preview-171--nextjs.preview.vtex.app
🔬   Go deeper by inspecting the Build Logs
📝   based on commit 2cfbe7a

@lucasfp13 lucasfp13 marked this pull request as ready for review July 15, 2022 22:15
@vtex-sites
Copy link

vtex-sites bot commented Jul 15, 2022

Lighthouse Reports

Here are the Lighthouse reports of this Pull Request

📝 Based on commit 2cfbe7a

Lighthouse Report by page
📎   /
📎   /apple-magic-mouse/p
📎   /office

Copy link
Contributor

@ArthurTriis1 ArthurTriis1 left a comment

Choose a reason for hiding this comment

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

Good! I left just one comment. But we won't add a story for Skeleton?

@lucasfp13
Copy link
Contributor Author

lucasfp13 commented Jul 19, 2022

Good! I left just one comment. But we won't add a story for Skeleton?

We will, I'm already working on them 🔛

@lucasfp13 lucasfp13 force-pushed the feat/FS-269-add-new-tokens-to-skeleton branch from 4002844 to 3fbd4f9 Compare July 19, 2022 18:00
@vercel vercel bot temporarily deployed to Preview July 19, 2022 18:01 Inactive
Copy link
Member

@eduardoformiga eduardoformiga left a comment

Choose a reason for hiding this comment

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

Amazing Job, @lucasfp13!
You also included ProductShelf 🎉

Sometimes I'm in doubt if we need some tokens, especially the ones with width or height with 100% or 0 values.


# Skeletons

This feature improves the store's experience and perceived performance through loading placeholders.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we classifying skeletons components as a feature?

Copy link
Member

Choose a reason for hiding this comment

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

Nice question! I also wonder about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discussed this with @renatamottam, and it makes sense to be a feature for her, also I agree that it is ok so then we group all skeletons

@lucasfp13 lucasfp13 force-pushed the feat/FS-269-add-new-tokens-to-skeleton branch from 4ea3489 to f80fcdb Compare July 21, 2022 03:09
@vercel vercel bot temporarily deployed to Preview July 21, 2022 03:11 Inactive
@lucasfp13 lucasfp13 force-pushed the feat/FS-269-add-new-tokens-to-skeleton branch from 0da3e8c to bb90f0e Compare July 21, 2022 17:32
@vercel vercel bot temporarily deployed to Preview July 21, 2022 17:34 Inactive
@vercel vercel bot temporarily deployed to Preview July 21, 2022 19:38 Inactive
@vercel vercel bot temporarily deployed to Preview July 22, 2022 00:06 Inactive
@vercel vercel bot temporarily deployed to Preview July 22, 2022 00:18 Inactive
@lucasfp13 lucasfp13 force-pushed the feat/FS-269-add-new-tokens-to-skeleton branch from afb3dc7 to 914fd26 Compare July 24, 2022 00:28
@vercel vercel bot temporarily deployed to Preview July 24, 2022 00:32 Inactive
@vercel vercel bot temporarily deployed to Preview July 25, 2022 13:09 Inactive
Copy link
Collaborator

@renatamottam renatamottam left a comment

Choose a reason for hiding this comment

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

LGTM!

@lucasfp13 lucasfp13 merged commit 81b2cbc into main Jul 25, 2022
@lucasfp13 lucasfp13 deleted the feat/FS-269-add-new-tokens-to-skeleton branch July 25, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Features New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants