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.

Sign up for GitHub

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

Jump to bottom

Task: Add redirect for popup #653

Merged

andrico1234 merged 5 commits into openui:main from andrico1234:feature/add-redirect

Jan 19, 2023

Collaborator

andrico1234 commented Jan 12, 2023 •

edited

Loading

Resolves: #634

Note: For some reason Yarn upgraded itself when I ran it on my machine. Would someone be able to pull my branch and run Yarn as well? I'm curious to see if it's going to cause issues on other devices

andrico-anima added 3 commits

January 12, 2023 21:11


          add redirect for popup

89074e6


          refactor code

c6c0026


          let yarn do its thing

952b6ec

andrico1234 changed the title ~~add redirect for popup~~ Task: Add redirect for popup

Collaborator Author

andrico1234 commented Jan 13, 2023

@mfreed7 @gregwhitworth

This one's ready for review + testing

Note: For some reason Yarn upgraded itself when I ran it on my machine. Would someone be able to pull my branch and run Yarn as well? I'm curious to see if it's going to cause issues on other devices

andrico1234 marked this pull request as ready for review

January 13, 2023 07:36

mfreed7 approved these changes

View reviewed changes

Collaborator

mfreed7 left a comment

Thanks for doing this!

Looks good to me. Thanks for the general cleanup. I added a few comments - generally I'm great with the cleanup, but I did disagree with a few of them.

You might also mention the cleanup in the commit description.

research/src/pages/components/popup.research.explainer.js Outdated

+              /**
+               *
+               * This component acts as a redirect to ensure backlinks aren't broken.

Collaborator

mfreed7 Jan 13, 2023

I suppose you could add a comment like

* This component acts as a redirect (from [1] to [2]) to ensure backlinks aren't broken.
* [1] https://open-ui.org/components/popup.research.explainer
* [2] https://open-ui.org/components/popover.research.explainer

research/src/pages/popup/popover.research.explainer.mdx Outdated


		This document proposes a set of APIs to make this type of UI easy to build.

Collaborator

mfreed7 Jan 13, 2023

This looks better.

research/src/pages/popup/popover.research.explainer.mdx Outdated

               ```javascript
               // These set the IDREF for the target element, and not the element itself:
-              myButton.popoverToggleTarget = idref;

Collaborator

mfreed7 Jan 13, 2023

Any reason to remove the ;? I've always thought JS's optional ; was a huge mistake.

research/src/pages/popup/popover.research.explainer.mdx Outdated

               ### Page Load Trigger
               As mentioned above, a `<div popover>` will be hidden by default. If it is desired that the popover should be shown automatically upon page load, the `defaultopen` attribute can be applied:
               ```html
-              <div popover defaultopen>
+              <div popover defaultopen></div>

Collaborator

mfreed7 Jan 13, 2023

Really, the whole defaultopen section should be removed. But ok for now.

research/src/pages/popup/popover.research.explainer.mdx Outdated

-                const testElement = document.createElement('div');
-                testElement.popover = type;
-                return testElement.popover == type.toLowerCase();
+                if (!HTMLElement.prototype.hasOwnProperty('popover')) return false // Popover API not supported

Collaborator

mfreed7 Jan 13, 2023

Disagree with this one too - having a control flow statement that far from the front of the line is a bug waiting to happen.

research/src/pages/popup/popover.research.explainer.mdx Outdated

               ```html
               <my-popover>
-                <template shadowroot=closed>
-                  <div popover=auto>This is a popover: <slot></slot></div>
+                <template shadowroot="closed">

Collaborator

mfreed7 Jan 13, 2023

Given recent declarative shadow dom conversations, sounds like we should make this <template shadowrootmode="closed">. But again, for another day.

research/src/pages/popup/popover.research.explainer.mdx Outdated

-              <popup role=list>
-              ...etc...
+              <popup role="dialog">
+                <popup role="list">

Collaborator

mfreed7 Jan 13, 2023

I think maybe this should not be nested:

<popup role=dialog></popup>
<popup role=list></popup>

Collaborator Author

andrico1234 commented Jan 15, 2023 •

edited

Loading

@mfreed7 , thanks for reviewing the PR

As for as the changes to "popover.research.explainer.mdx" go, I have a feeling that Prettier automatically formatted the file when I made a change to it. I'll revert any formatting only changes to reduce the change surface of this change

EDIT:

looks like the repo is designed to run prettier during the pre-commit hook, which explains the formatting change. From the looks of it, the prettierconfig.json is set to remove semi-colons.

It might be best to either:

disable it for mdx files
run it against MDX files with the current settings

We don't have to come to a decision now, but I've reverted any formatting only change for the sake of this PR

andrico1234 added 2 commits

January 15, 2023 11:52


          revert formatting change


          add redirect comment

fd28101

andrico1234 merged commit 2faacc2 into openui:main

gregwhitworth added a commit that referenced this pull request


          [Site rewrite] Migrate site to astro (#606)

* initial astro commit

* minor config change

* move over sources

* fix prettier formatting

* rename files and create layout

* move to markdown

* migrate markdown file

* get navigation working

* fix routing for pages

* update the documentation

* more fixes to styling

* get layouts working for all pages

* improve styles even more

* get build working

* fix image

* fix navigation order

* get navigation working

* add favicon

* fix page title

* fix menu sorting

* fix lighthouse issues

* update node version

* minors to styles

* override gatsby build config

* typo

* fix navigation styles

* remove unused dependencies, remove render blocking resources, prefetch resources

* reduce font flashing, fix markup issues

* fix build from breaking

* test removing prefetch

* rmeove compression

* revert changes to config

* fix prefetch

* fix image paths

* Create 2022-12-01.md

* Update 2022-12-01.md

* Update 2022-12-01.md

* Update popup.research.explainer.mdx (#644)

Remove lingering mention of `popover=hint` for now.

* Popover target attribute updates (#647)

The `popovertoggletarget`, `popovershowtarget` and `popoverhidetarget` attributes will initially map to `aria-expanded`.

This has already been implemented in Chromium browsers.  HTML AAM PR to add this mapping to the spec: w3c/html-aam#446

* Create 2023-01-05.md

* Update 2023-01-05.md

* Create 2023-01-12.md

* docs: add kolibri accessible component lib (#624)

* Update 2023-01-12.md

* Create 2023-01-19.md

* Task: Add redirect for popup (#653)

* add redirect for popup

* refactor code

* let yarn do its thing

* revert formatting change

* add redirect comment

Co-authored-by: Andrico <andrico@animaapp.com>

* Update popover.research.explainer.mdx (#654)

* Update popover.research.explainer.mdx

* Update research/src/pages/popup/popover.research.explainer.mdx

Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>

Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>

* Create 2023-02-02.md

* fix layout issue

* fix interactions

* Update 2023-02-02.md

* Update 2023-02-02.md

* selectmenu is an element, code escape it (#660)

* update layout and remove stale content

* handle missing images

* minor fixes and redirects

* fix dead links

* Create 2023-02-16.md

* Update 2023-02-16.md

* Adjusted the home page to reflect the current reality and ensure the relationship between Open UI & the W3C is clear (#663)

* Create 2023-02-23.md

* minor improvements

* temporarily prevent refetch

* revert prefetch change

* Update 2023-02-23.md

* performance + a11y improvements

* remove Lora font family

---------

Co-authored-by: Andrico <andrico.karoulla@healthhero.com>
Co-authored-by: Greg Whitworth <865244+gregwhitworth@users.noreply.github.com>
Co-authored-by: Jhey Tompkins <jh3y@users.noreply.github.com>
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
Co-authored-by: Martin <6279703+deleonio@users.noreply.github.com>
Co-authored-by: Andrico <andrico@animaapp.com>
Co-authored-by: Tantek Çelik <blog@tantek.com>

gregwhitworth mentioned this pull request

[Site rewrite] Migrate site to astro (#606) #667

Merged

gregwhitworth added a commit that referenced this pull request


          [Site rewrite] Migrate site to astro (#606) (#667)

1fef10f

* [Site rewrite] Migrate site to astro (#606)

* initial astro commit

* minor config change

* move over sources

* fix prettier formatting

* rename files and create layout

* move to markdown

* migrate markdown file

* get navigation working

* fix routing for pages

* update the documentation

* more fixes to styling

* get layouts working for all pages

* improve styles even more

* get build working

* fix image

* fix navigation order

* get navigation working

* add favicon

* fix page title

* fix menu sorting

* fix lighthouse issues

* update node version

* minors to styles

* override gatsby build config

* typo

* fix navigation styles

* remove unused dependencies, remove render blocking resources, prefetch resources

* reduce font flashing, fix markup issues

* fix build from breaking

* test removing prefetch

* rmeove compression

* revert changes to config

* fix prefetch

* fix image paths

* Create 2022-12-01.md

* Update 2022-12-01.md

* Update 2022-12-01.md

* Update popup.research.explainer.mdx (#644)

Remove lingering mention of `popover=hint` for now.

* Popover target attribute updates (#647)

The `popovertoggletarget`, `popovershowtarget` and `popoverhidetarget` attributes will initially map to `aria-expanded`.

This has already been implemented in Chromium browsers.  HTML AAM PR to add this mapping to the spec: w3c/html-aam#446

* Create 2023-01-05.md

* Update 2023-01-05.md

* Create 2023-01-12.md

* docs: add kolibri accessible component lib (#624)

* Update 2023-01-12.md

* Create 2023-01-19.md

* Task: Add redirect for popup (#653)

* add redirect for popup

* refactor code

* let yarn do its thing

* revert formatting change

* add redirect comment

Co-authored-by: Andrico <andrico@animaapp.com>

* Update popover.research.explainer.mdx (#654)

* Update popover.research.explainer.mdx

* Update research/src/pages/popup/popover.research.explainer.mdx

Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>

Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>

* Create 2023-02-02.md

* fix layout issue

* fix interactions

* Update 2023-02-02.md

* Update 2023-02-02.md

* selectmenu is an element, code escape it (#660)

* update layout and remove stale content

* handle missing images

* minor fixes and redirects

* fix dead links

* Create 2023-02-16.md

* Update 2023-02-16.md

* Adjusted the home page to reflect the current reality and ensure the relationship between Open UI & the W3C is clear (#663)

* Create 2023-02-23.md

* minor improvements

* temporarily prevent refetch

* revert prefetch change

* Update 2023-02-23.md

* performance + a11y improvements

* remove Lora font family

---------

Co-authored-by: Andrico <andrico.karoulla@healthhero.com>
Co-authored-by: Greg Whitworth <865244+gregwhitworth@users.noreply.github.com>
Co-authored-by: Jhey Tompkins <jh3y@users.noreply.github.com>
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
Co-authored-by: Martin <6279703+deleonio@users.noreply.github.com>
Co-authored-by: Andrico <andrico@animaapp.com>
Co-authored-by: Tantek Çelik <blog@tantek.com>

* remove unused dependencies

---------

Co-authored-by: Andrico <karoulla.andrico@gmail.com>
Co-authored-by: Andrico <andrico.karoulla@healthhero.com>
Co-authored-by: Jhey Tompkins <jh3y@users.noreply.github.com>
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
Co-authored-by: Martin <6279703+deleonio@users.noreply.github.com>
Co-authored-by: Andrico <andrico@animaapp.com>
Co-authored-by: Tantek Çelik <blog@tantek.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet