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

website: Update variables documentation & demos #1334

Merged
merged 22 commits into from
Jun 20, 2023

Conversation

FlyersPh9
Copy link
Collaborator

@FlyersPh9 FlyersPh9 commented Jun 7, 2023

Changes

  • Add, correct, & expand upon documentation text.
  • Add new demos for:
    • Durations
    • Component heights
  • Add table of contents to variables page.
  • Add link to https://unpkg.com/browse/@itwin/itwinui-variables/index.css for colleagues who want access to the variable values for mockups. This can possibly be removed when the last bullet point of Website: improvement #941 is addressed.
  • Reorder sections to be alphabetical. This will be done after PR is approved to keep reviewing as easy as possible.

@FlyersPh9 FlyersPh9 added the documentation Improvements or additions to documentation label Jun 7, 2023
@FlyersPh9 FlyersPh9 requested a review from a team as a code owner June 7, 2023 17:34
@FlyersPh9 FlyersPh9 self-assigned this Jun 7, 2023
@FlyersPh9 FlyersPh9 requested a review from a team as a code owner June 7, 2023 17:34
@FlyersPh9 FlyersPh9 requested review from mayank99 and adamhock and removed request for a team June 7, 2023 17:34
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This demo is a bit intense. Suggestions for an alternative demo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a gentle fade?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realized my animation was incorrect. It was applying the duration speed to expand AND collapse. I've added animation-direction: alternate; so now the animation is half of what it was. Still a bit intense, but better than before.

I will look into other options next time I work on this PR, with the time I have today this is the best I could do.

Copy link
Contributor

Choose a reason for hiding this comment

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

A non-repeating animation that can be triggered by the user would be the best option. For inspiration, see https://open-props.style/#animations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to require a user's click to trigger animation.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm it's not working for me. did you forget to add a script?

Screen.Recording.2023-06-15.at.1.44.50.PM.mov

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 fix it using this code:

DurationDemo.astro
---
import Wrapper from './_wrapper.astro';
const durations = ['0', '1', '2', '3'];
---

<Wrapper class='wrapper'>
  {
    durations.map((duration) => (
      <duration-item duration={`--iui-duration-${duration}`}>
        <button>{duration}</button>
      </duration-item>
    ))
  }
</Wrapper>

<script>
  customElements.define(
    'duration-item',
    class extends HTMLElement {
      connectedCallback() {
        const duration = getComputedStyle(this).getPropertyValue(this.getAttribute('duration'));
        const durationMs = Math.max(0.001, parseFloat(duration) * 1000);
        const button = this.querySelector('button');

        button.addEventListener('click', () => {
          button.animate([{ opacity: 0 }], { duration: durationMs });
        });
      }
    }
  );
</script>

<style>
  duration-item button {
    border: none;
    padding: var(--space-3);
    width: var(--space-8);
    height: var(--space-8);
    border-radius: var(--space-1);
    box-shadow: 0 0 4px hsl(0 0% 0% / 20%);
    display: flex;
    align-items: center;
    justify-content: center;
    background-color: var(--iui-color-background);
    color: var(--iui-color-foreground-2);
    cursor: pointer;
  }
</style>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just tested it in Brave & Safari, those both seems to be working.
duration-demo

Firefox however is not working. 😫

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you, the suggested code works in all browsers!

@@ -27,56 +31,45 @@ import '@itwin/itwinui-variables';

## Variables

### Background colors
We strongly encourage developers to use iTwinUI's CSS variables rather than hardcoding values. Many of our color values change depending on the theme or contrast settings. Because of this, we purposefully do not expose the variables' values on this site. However, if you need the variables values you can extrapolate those values from [the raw CSS file](https://unpkg.com/browse/@itwin/[email protected]/index.css).
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be better to link to https://unpkg.com/@itwin/itwinui-variables/index.css as it will always redirect to latest version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I linked to https://unpkg.com/browse/@itwin/[email protected]/index.css on purpose incase we were to introduce iTwinUI-Variables 3.x and we wanted to version our documentation site. But since we haven't introduced versioning to the doc site yet, maybe I'm thinking too far ahead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Linking to https://unpkg.com/@itwin/itwinui-variables/index.css now that this branch is targeting dev.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use https://cdn.jsdelivr.net/npm/@itwin/itwinui-variables/index.css instead? every day i become less confident in unpkg because i'm seeing increasing errors and slowness

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, using this link instead.

apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
@mayank99
Copy link
Contributor

mayank99 commented Jun 7, 2023

Should target dev branch, not main.

@gretanausedaite
Copy link
Contributor

Should target dev branch, not main.

Should target main branch as we want to deploy it sooner than v3 changes comes. This update is needed for UX designers.

@mayank99
Copy link
Contributor

mayank99 commented Jun 8, 2023

Should target dev branch, not main.

Should target main branch as we want to deploy it sooner than v3 changes comes. This update is needed for UX designers.

It does not make any sense to maintain two diverging codebases with different versions of documentation when neither of them is close to finished. When we started work on v3, we decided that we will only focus on documenting the latest code. Why complicate things now and create additional work? We can give DEV url to anyone who wants to see these updates.

mayank99 added a commit that referenced this pull request Jun 8, 2023
Copy link
Contributor

@adamhock adamhock left a comment

Choose a reason for hiding this comment

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

A few grammar comments, but overall looks good!

apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
@FlyersPh9 FlyersPh9 changed the base branch from main to dev June 14, 2023 20:16
apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
apps/website/src/pages/docs/variables.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -17,6 +17,12 @@ iTwinUI offers an extensive set of handcrafted CSS variables. If you're using iT

We strongly encourage the use iTwinUI's CSS variables rather than hardcoding values. Many of the color values change depending on the theme or contrast settings. Because of this, we purposefully do not expose the variables' values on this site. However, if you need a particular variable's value, you can extrapolate it from [the raw CSS file](https://cdn.jsdelivr.net/npm/@itwin/itwinui-variables/index.css).

## Border radius
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the benefit of alphabetically sorting? to me, it makes more sense to sort in a logical order / by importance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can take a guess as the order of importance, but alphabetical at least makes scanning the TOC easier. We have the left side nav sorted alphabetically as well.

Copy link
Contributor

@mayank99 mayank99 Jun 16, 2023

Choose a reason for hiding this comment

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

It's probably subjective but I would also like to reorder the left side nav a bit more logically - currently it can be a bit hard to find components. I think I would like to group the components into sections and have components within each section sorted alphabetically, and the sections themselves sorted logically

But we can punt on this decision and discuss it further internally, with some research and a vote. I'm ok with merging as-is for now.

cc @gretanausedaite

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with discussing it further internally.

@FlyersPh9 FlyersPh9 added this pull request to the merge queue Jun 16, 2023
@FlyersPh9 FlyersPh9 removed this pull request from the merge queue due to a manual request Jun 16, 2023
@FlyersPh9 FlyersPh9 added this pull request to the merge queue Jun 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 16, 2023
@FlyersPh9 FlyersPh9 added this pull request to the merge queue Jun 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 20, 2023
@FlyersPh9 FlyersPh9 disabled auto-merge June 20, 2023 16:53
@FlyersPh9 FlyersPh9 merged commit de9a782 into dev Jun 20, 2023
@FlyersPh9 FlyersPh9 deleted the jon/documentation-site-variables branch June 20, 2023 19:50
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants