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.

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

[WIP] PublicationMenu with PublicationInfoDialog + DeletePublicationAlertDialog | publicationOpdsInfo | publicationInfoReader #2016

Closed
wants to merge 79 commits into from
Closed

[WIP] PublicationMenu with PublicationInfoDialog + DeletePublicationAlertDialog | publicationOpdsInfo | publicationInfoReader #2016

wants to merge 79 commits into from

Conversation

panaC
Copy link
Member

@panaC panaC commented Sep 29, 2023

No description provided.

@panaC panaC self-assigned this Sep 29, 2023
@socket-security
Copy link

socket-security bot commented Sep 29, 2023 •
edited
Loading

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@radix-ui/react-popover 1.0.7 None +9 692 kB benoitgrelard
@radix-ui/react-alert-dialog 1.0.5 None +0 112 kB benoitgrelard

@panaC
Copy link
Member Author

panaC commented Oct 2, 2023

new menu with focus lock and Radix&floating-UI :

image

PublicationInfo with Dialog Radix-UI :
image

Delete Altert Modal with Radix :

image

The ESC chain work.

Same thing on Opds section :

image

Need to fix the css dimensions of the publication Modal

@panaC panaC mentioned this pull request Oct 6, 2023
Closed
@panaC
Copy link
Member Author

panaC commented Nov 30, 2023

image

nitpicking many details need to be checked yet :)
bold title / bullet point / lcp columns / margins

panaC
panaC commented Dec 4, 2023
View reviewed changes
src/renderer/common/components/dialog/publicationInfos/publicationInfoA11y.tsx Outdated
: <></>}
{(a11y_certifiedBy || a11y_certifierCredential || AccessibilityFeature || AccessibilityConformsTo || AccessibilityConformanceReport || AccessibilitySummary) ?
<div className={stylePublication.accessibility_infos_right}>
{/* <details> */}
Copy link
Member Author

Choose a reason for hiding this comment

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

@danielweck Just a question : there are no accessibility issue to remove details and summary html elements ?

Copy link
Member

Choose a reason for hiding this comment

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

The HTML summary/detail elements provide built-in interaction behaviour and semantics, compared with div, and should be easier to style too. Better avoid generic HTML containers like div when there is a semantically-stronger alternative in the web standard.

@panaC
Copy link
Member Author

panaC commented Dec 6, 2023 •
edited
Loading

some points to check :

  • modal width size is not fixed until window size is less than its value
    image
    image

  • we need to update the font size in the figma model to be related to the thorium implementation

  • some margins seems to tight like the distance between tag label and tag items
    image

  • columns implemented for a11y but not for lcp

image

  • the width of the reader window too small compared to the size of the modal (broken also on the dev branch)
    image
    image

  • spine items in readerWIndow need some improvement to fit figma design (arrow and margins)
    image
    image

seems very cool, we are really close to being able to merge into develop :)

@panaC
Copy link
Member Author

panaC commented Dec 6, 2023

@arthur-lemeur @llemeurfr @danielweck The question arises whether we should not increase the min width and height of a window

thorium-reader/src/main/redux/sagas/win/browserWindow/createReaderWindow.ts

Lines 35 to 36 in 9dd2e12

minWidth: 600,
minHeight: 400,

thorium-reader/src/main/redux/sagas/win/browserWindow/createLibraryWindow.ts

Lines 46 to 47 in 9dd2e12

minWidth: 800,
minHeight: 600,

@panaC
Copy link
Member Author

panaC commented Jan 5, 2024

i'm closing it, merged to ui master ui branch

@panaC panaC closed this Jan 5, 2024
@danielweck danielweck deleted the menu-radix-pubInfo branch April 25, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielweck danielweck danielweck left review comments

@llemeurfr llemeurfr Awaiting requested review from llemeurfr

@gautierchomel gautierchomel Awaiting requested review from gautierchomel

@arthur-lemeur arthur-lemeur Awaiting requested review from arthur-lemeur

Assignees

@panaC panaC

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@panaC @danielweck @arthur-lemeur