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(about): View About info from PageSideBar #299

Merged
merged 6 commits into from
Sep 28, 2021

Conversation

jan-law
Copy link
Contributor

@jan-law jan-law commented Sep 27, 2021

Fixes #266.

Would appreciate some suggestions to improve this design since my current approach causes the bug listed below.

  1. In addition to the existing masthead help icon, clicking the "About" nav link on the PageSideBar will display the AboutModal. Unlike the other nav links, users can view the AboutModal by clicking the nav link without logging in.

  2. The url http://localhost:9000/about allows users to view/copy a link to the AboutModal. Followed this example to create the modal route: https://reactrouter.com/web/example/modal-gallery
    Note: The example only displays their modal when the route matches “/img/:id”. Since I wanted to display the modal on top of any page, the RouteWithTitleUpdates``path prop is set to location.pathname

Bug: if you open a new tab and enter http://localhost:9000/recordings/about, the modalBackground - which renders the previous web page content in the background - is undefined. The <Switch> will then render the NotFound page. If I instead replace the RouteWithTitleUpdates``path prop to /about on line 204 and comment lines 146-148 in AppLayout, the previous page content does not render in the background behind the modal.

image

@jan-law jan-law changed the title feat(about-modal): View AboutModal from PageSideBar feat(about): View AboutModal from PageSideBar Sep 27, 2021
@jan-law jan-law force-pushed the promote-about-modal branch from 65398c6 to c9f2809 Compare September 27, 2021 18:30
@aptmac
Copy link
Member

aptmac commented Sep 27, 2021

1. In addition to the existing masthead help icon, clicking the "About" nav link on the PageSideBar will display the AboutModal. Unlike the other nav links, users can view the AboutModal by clicking the nav link without logging in.

What if instead of the nav-bar opening the about modal itself, the about-modal design was extracted into it's own component such that it could be used as a dedicated "about" page and also be fed into the current about-modal?

Bug: if you open a new tab and enter http://localhost:9000/recordings/about, the modalBackground - which renders the previous web page content in the background - is undefined. The <Switch> will then render the NotFound page. If I instead replace the RouteWithTitleUpdatespath `` prop to /about on line 204 and comment lines 146-148 in `AppLayout`, the previous page content does not render in the background behind the modal.

This looks to be a routing issue; there isn't a defined route to /<whatever>/about. What if instead of adding "/about" to the end of the URL, a URL fragment is used (e.g., #about)? So now if the user tries going to /recordings#about then we first route to recordings and then open the about modal, instead of trying to route to /recordings/about.

@jan-law
Copy link
Contributor Author

jan-law commented Sep 27, 2021

the about-modal design was extracted into it's own component such that it could be used as a dedicated "about" page and also be fed into the current about-modal

Nice, the About page also prevents the need to create extra routes or URL fragments.

@jan-law jan-law changed the title feat(about): View AboutModal from PageSideBar feat(about): View About info from PageSideBar Sep 27, 2021
@jan-law jan-law marked this pull request as ready for review September 27, 2021 20:53
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

I think this looks pretty good. As a follow-up, could you look into navigation item groups or separators and see what improvements can be made there? https://www.patternfly.org/v4/components/navigation/

@andrewazores
Copy link
Member

@aptmac any comments on these latest commits?

Copy link
Member

@aptmac aptmac 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!

@andrewazores andrewazores merged commit 69d0743 into cryostatio:main Sep 28, 2021
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.

See if About Modal can be made more discoverable
3 participants