-
Notifications
You must be signed in to change notification settings - Fork 7
Libre311 Accessibility Issue #38 #338
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
Conversation
List View - There is only one way to get to this page, the "List" button. This addressed how this was implemented by creating a different route to the list. Signed-off-by: Tim Dalton <daltont@objectcomputing.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was duplicate code
| export let sideBarHidden = false; | ||
| export let mainContentHidden = false; | ||
| $: layoutContainerClass = `layout-container-${sideBarHidden}-${mainContentHidden}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this approach works, but it results in CSS class names that aren't very meaningful. How about something like this?
$: layoutContainerClass = listHidden ? 'map-only' : mapHidden ? : list-only' ? 'list-and-map';This assumes that the variable sideBarHidden is renamed to listHidden and mainContentHidden is renamed to mapHidden.
The CSS class names would have to be changed to match the names used above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commited fix.
Originally had this idea of keeping the SideBarMainContentLayout as a generic resusable thing and with side-bar / main-content, but went ahead and renamed the component.
| .layout-container-false-true { | ||
| display: grid; | ||
| grid-template-columns: auto; | ||
| height: 100%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like height: 100% is always what is desired and shouldn't need to be specified for all three cases.
| } | ||
| aside { | ||
| .side-bar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this CSS class should be named list-view to make it more clear.
…ccessibility issue #66. Signed-off-by: Tim Dalton <daltont@objectcomputing.com>
Signed-off-by: Tim Dalton <daltont@objectcomputing.com>
Signed-off-by: Tim Dalton <daltont@objectcomputing.com>
List View - There is only one way to get to this page, the "List" button.
This addressed how this was implemented by creating a different route to the list.