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

Copy popup explainer from mfreed7 repo #490

Merged
merged 5 commits into from
Mar 18, 2022
Merged

Conversation

mfreed7
Copy link
Collaborator

@mfreed7 mfreed7 commented Mar 16, 2022

This is a (mostly) copy/paste from my personal repo into the OpenUI repo. I updated the other popup-related explainers to have a pointer to this new approach.

I've left these as .MD files, since there's nothing but pure Markdown. Let me know if I need to rename them to mdx.

Once this lands, I'll add an "obsolete" message at the top of my personal repo documents, pointing to this copy.

Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

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

I left a comment as I'd like the proposal link on the left side to link to the actual current proposal which is your new.popup.explainer.md rather than updating the current proposal to link to the new one.

Additionally, the routing seems to not be working for your file in the preview

@andrico1234
Copy link
Collaborator

Looking at the preview it I don't see a link to the new popup proposal in the sidebar:

image

The link to the new page returns a 404 too,
https://deploy-preview-490--open-ui.netlify.app/components/new.popup.explainer.md

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 17, 2022

Looking at the preview it I don't see a link to the new popup proposal in the sidebar:

image

The link to the new page returns a 404 too, https://deploy-preview-490--open-ui.netlify.app/components/new.popup.explainer.md

Right - I'm completely unfamiliar with the setup of the OpenUI site. I'll try to figure it out. Any clues would be appreciated!

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 17, 2022

Alright, I think I figured things out and relinked/renamed things appropriately. Please take a look and let me know if you see any broken links in the preview. They looked good to me.

I ended up keeping the original link (https://open-ui.org/components/popup.research.explainer) for the new explainer, and renaming the original things to -v1. That way I don't break existing links from elsewhere. LMK if you'd like something different!

@gregwhitworth
Copy link
Member

Thanks for making the changes.

One minor change if we're going to link to both let's adjust the terminology of the naming:

  • Popup API (Explainer)
  • Popup Element (Abandoned)

@andrico1234
Copy link
Collaborator

Retested and works as expected! 😄

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Mar 17, 2022

Thanks for making the changes.

One minor change if we're going to link to both let's adjust the terminology of the naming:

  • Popup API (Explainer)
  • Popup Element (Abandoned)

Thanks for the review. No problem on renaming - done. PTAL and let me know if this looks like what you were expecting.

Retested and works as expected! 😄

Great, thanks for looking!

Anything else you all see that I need to change, or is this ok to land? (I'd like to send an updated Intent to Prototype, and I need to link to this explainer.)

@christopherallanperry
Copy link
Collaborator

Looks right to me.

@gregwhitworth gregwhitworth merged commit c220a42 into openui:main Mar 18, 2022
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.

4 participants