Add live region for navigation changes #2214
Conversation
RoyEJohnson
force-pushed
the
add-live-region-for-navigation-changes
branch
from
33b0307 to
bf1cf4f
Compare
src/app/components/NavBar/index.tsx
Outdated
| @@ -134,6 +134,7 @@ const NavigationBar = ({user, loggedOut, currentPath}: NavigationBarProps) => | |||
| </a> | |||
| {loggedOut && <LoggedOutState currentPath={currentPath} />} | |||
| {user && <LoggedInState user={user} currentPath={currentPath} />} | |||
| <Styled.PageTitleConfirmation /> | |||
There was a problem hiding this comment.
does it matter where in the document you put this? it doesn't read it in order or anything right? maybe put it up the tree a bit in Layout or Content or something? NavBar seems a bit weird
There was a problem hiding this comment.
Moved to Layout
| className="c13" | ||
| id="page-title-confirmation" | ||
| > | ||
| Loaded page "" |
There was a problem hiding this comment.
does this read out on page load? would we prefer that it be empty at first instead of having "loaded page [silence]"?
There was a problem hiding this comment.
I have it skip the first non-blank value
| PageTitleConfirmation | ||
| } from './PageTitleConfirmation'; | ||
|
|
||
| describe('On scroll', () => { |
| fn?: (title: string) => void; | ||
| }; | ||
|
|
||
| const announcePageTitle: FunctionWithProperty = (title: string) => { |
There was a problem hiding this comment.
i think this component should work based on being connected to the redux state instead of having a separate callback like this
RoyEJohnson
force-pushed
the
add-live-region-for-navigation-changes
branch
from
d88c138 to
45c85ab
Compare
Skip announcing initial page title
| ); | ||
| } | ||
|
|
||
| export default connect(({ head: { title } }: AppState) => ({ title }))(styled( |
There was a problem hiding this comment.
there is a selector for the title https://github.com/openstax/rex-web/blob/main/src/app/head/selectors.ts#L19
using it will affect nobody's life but it follows the convention. when you have derived values in selectors it does make a difference because they're memoized based on the input state and will help reduce react re-renders
There was a problem hiding this comment.
I added a auto-focus to main when its content changes, and also revised this to use the selector.
|
@RoyEJohnson can you remove the focus on main thing and we'll get the announcement merged? i have concerns about when the content is changed as part of a navigation with a target, like clicking a search result or a link with a hash |
RoyEJohnson
force-pushed
the
add-live-region-for-navigation-changes
branch
from
1a9a8b1 to
15f6828
Compare
|
There is an existing issue for focusing on main when content changes. |
DISCO-187
Adding you as a reviewer, Tom, because I suspect there are decisions I made that are open to criticism.