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: Temporary placeholders for unfinished pages on doc website #1099

Merged
merged 9 commits into from
Mar 8, 2023

Conversation

elephantcatdog
Copy link
Contributor

@elephantcatdog elephantcatdog commented Feb 24, 2023

Changes

I thought it would be helpful to have placeholders for unfinished pages on the doc website so that people going here or being redirected here from ux.bentley.com aren't only getting a bunch of mostly empty pages with little information on them.

I want to add it to all the unfinished pages, but wanted to see if this format/wording is good before copy/pasting to all the other pages. I added the lines before and after so that it's a bit more noticeable. Also I'm not sure if it's better to have the full link be visible vs just having 'storybook' with a link.

image

Testing

N/A

Docs

N/A

@elephantcatdog elephantcatdog requested a review from a team as a code owner February 24, 2023 22:59
@elephantcatdog elephantcatdog requested review from a team, gretanausedaite, adamhock, FlyersPh9 and mayank99 and removed request for a team February 24, 2023 22:59
Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Very good idea!
Maybe we could add some kind of banner/alert instead of simple text? You could create a component and pass down all mdx pages that need this info. Also, you could create new styling for this component, it doesn't have to be iTwinUI - you can use our theme gradients or something.

@mayank99
Copy link
Contributor

Agreed with @gretanausedaite

@elephantcatdog
Copy link
Contributor Author

elephantcatdog commented Feb 28, 2023

I'm not sure why, but the grid template isn't working correctly for the alert I added:
image

I followed how it was done for the other elements:
image
It works for the regular grid-template but not for the @media version. It's as if I wrote 'beforemain . . toc' auto instead.

This was supposed to be a quick addition, so I didn't make the alert look fancy or anything.

Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Grid does not work because Alert does not get "placeholder" class.
Add classname prop (eg. Card.astro) or move styles to Placeholder.astro.
Also I think it'd look better if alert had space before title (bottom padding) (eg. var(--space-5)).

You sure you don't want to play around with styles?
image

<span class='iui-alert-message'
>Some pages have not yet been finished and are being worked on. In the meantime, visit <a
class='iui-alert-link'
href='https://itwin.github.io/iTwinUI/react/?path=/story/overview--page'>storybook</a
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cool if we routed users to same component in storybook. Can be added later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@mayank99
Copy link
Contributor

mayank99 commented Mar 1, 2023

I think it might be better to control this message at a page-level rather than adding to the grid on all pages.

This will also allow customizing the message/link for each page.

The implementation could look like one of these two options:

  • A simple component that takes the component name (or url) as prop.

    ---
    title: Alert
    ...
    ---
    ...
    <PlaceholderMessage component='alert' />
  • Alternatively, use a frontmatter property to indicate that the page should have a message.

    ---
    title: Alert
    ...
    wip: true
    ---
    ...

    After this, you'll be able to access this property (among other properties such as title) in the layout and decide what to show.

@elephantcatdog
Copy link
Contributor Author

elephantcatdog commented Mar 6, 2023

@mayank @gretanausedaite

Thank you both for your input! I've updated the placeholder. Now it's got a property for the end of the storybook url, and is placed on an individual component's page vs the layout. I did it this way because the end of the urls vary a bit. I also updated the visuals to something similar to what you showed, Greta, because I thought that looked nice.

If this looks good, then I'll put the placeholder on all the pages that need it. I have it on 'Avatar' and 'Carousel' right now (and will remove it from 'Avatar').

image

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.

usage looks good so i think it can be added to all pages as-is, but i think the styling could be toned down to be less distracting.

i also wonder if the note should come before or after the component description. probably after? because the description is closely connected with the heading.

apps/website/src/components/Placeholder.astro Outdated Show resolved Hide resolved
apps/website/src/components/Placeholder.astro Outdated Show resolved Hide resolved
apps/website/src/components/Placeholder.astro Outdated Show resolved Hide resolved
@elephantcatdog
Copy link
Contributor Author

elephantcatdog commented Mar 7, 2023

Updated look: image

Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Agree with using variables instead of hardcoding hex.

@veekeys
Copy link
Member

veekeys commented Mar 7, 2023

Updated look: image

Would it be possible to get rid of any CONNECT related references in public docs site? I do understand initially we might have copied some of the old content, but in this particular case (hopefully not that many in all docs site) it is something we do not want to even mention to anyone anymore.

@mayank99
Copy link
Contributor

mayank99 commented Mar 7, 2023

Would it be possible to get rid of any CONNECT related references in public docs site? I do understand initially we might have copied some of the old content, but in this particular case (hopefully not that many in all docs site) it is something we do not want to even mention to anyone anymore.

Agreed we should remove that.

Almost every single page had content copied over from the old docs, so there might be more weird stuff. #961 is tracking the status of pages that have been further reviewed/adjusted after the old content was moved.

Copy link
Collaborator

@FlyersPh9 FlyersPh9 left a comment

Choose a reason for hiding this comment

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

Once this comment is resolved, looks good to me!

@elephantcatdog
Copy link
Contributor Author

Okay, I added the placeholder to all the unfinished pages, and confirmed that the links go where they're supposed to. I have a question about one of the pages though - 'Typography' is a page that has 'text' and 'label' on it, but we have separate pages for the other types of typography. Why does this page exist as it is, and not either include all types of typography or have separate pages for all?

@gretanausedaite
Copy link
Contributor

I have a question about one of the pages though - 'Typography' is a page that has 'text' and 'label' on it, but we have separate pages for the other types of typography. Why does this page exist as it is, and not either include all types of typography or have separate pages for all?

What are "other types of typography"?

@elephantcatdog
Copy link
Contributor Author

What are "other types of typography"?

These are all listed under "Typography" in storybook:
image

@gretanausedaite
Copy link
Contributor

These are all listed under "Typography" in storybook: image

They are under "typography" in side nav.
image

We've just combined text and label because it would be hard to separate them visually in all components page
image

@elephantcatdog
Copy link
Contributor Author

Hm, I see, thanks Greta

@elephantcatdog elephantcatdog added this pull request to the merge queue Mar 8, 2023
Merged via the queue into main with commit 08e8973 Mar 8, 2023
@elephantcatdog elephantcatdog deleted the leah/storybook-redirect branch March 8, 2023 22:42
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.

5 participants