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

Add ADR 0040 - Display front cover for books in Preview Panel #12122

Merged

Conversation

damgam0288
Copy link
Contributor

@damgam0288 damgam0288 commented Oct 29, 2024

This PR is related to issue 10120. Following the comment discussion, we decided where to place the front cover for book citations in the UI, and this decision is documented in an ADR. The goal of this PR is to merge this ADR.

The ADR includes details on the proposed UI changes; the screenshots were not duplicated here to avoid redundancy and because they have not been implemented yet.

Mandatory checks

  • I own the copyright of the code submitted and I licence it under the MIT license
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

First comments on the "syntax", before I concentrate on the comments.


### 1. Existing SidePane

![Image: Placement in SidePane](https://github.com/user-attachments/assets/7f704b0c-6f0c-4501-8167-4dc6202ca8f6)
Copy link
Member

Choose a reason for hiding this comment

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

Please store the image in the repository, too.

Name it 0040-placement-sidepane.png. Do it for the others accordingly. (if it is jpg, use jpg as file extension)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I put it in the docs/decisions folder?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. With the filename prefix 0040-*, it will sorted next to the ADR.

Copy link
Member

Choose a reason for hiding this comment

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

@damgam0288 I am still waiting for the images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have uploaded them now

Copy link
Member

Choose a reason for hiding this comment

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

@damgam0288 You also need to change all image links. That means, instead of github.com ..., the respective file should be referenced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry! I missed that. Have updated now.


## Pros and Cons of the Options

### 1. Existing SidePane
Copy link
Member

Choose a reason for hiding this comment

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

Omit numbers in the headings. Remove them.


## Decision Outcome

Chosen option: "3. The PreviewPanel of the EntryEditor".
Copy link
Member

Choose a reason for hiding this comment

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

Remove 3.

### 2. New right-sided SidePane

* Good, if integrated together with entry preview because it would make it [easier to view a citation's preview](https://github.com/JabRef/jabref/issues/10120#issuecomment-2422099269).
* Bad, because an extra SidePane would [make the interface overly complex](https://github.com/JabRef/jabref/issues/10120#issuecomment-2422677378).
Copy link
Member

Choose a reason for hiding this comment

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

Place the image before this list. Also for the other images.

![Image: Placement in the Preview Panel](https://github.com/user-attachments/assets/68b9065b-bac6-412b-9815-7d27d2fbe0be)

* Good, because it would not be obtrusive or distracting.
* Bad, because users will have to click multiple times (enable preview tab, open entry editor) to see the book cover.
Copy link
Member

@calixtus calixtus Oct 30, 2024

Choose a reason for hiding this comment

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

I would not include a one-time configurable preference option here. Only if the default behaviour is a major problem. In that case, we should change the default behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

The "bad" describes "only" the case if users do not have the entry preview opened.

Suggested change
* Bad, because users will have to click multiple times (enable preview tab, open entry editor) to see the book cover.
* Bad, because coupled to the preview, which is coupled to the entry editor: In case the entry editor is closed, users will have to open the entry editor and navigate to the tab "Preview" or "Requried fields".

I think @damgam0288 did not see that the entry preview is embedded in the tab "Required fields"

image

@koppor koppor added the status: changes required Pull requests that are not yet complete label Oct 31, 2024
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

I renamed ADR-0040 in the main branch to ADR-0041 to ease merging (otherwise, we would need to rename all files here)

@koppor koppor added this pull request to the merge queue Nov 2, 2024
@koppor koppor removed the status: changes required Pull requests that are not yet complete label Nov 2, 2024
Merged via the queue into JabRef:main with commit 96f3bc3 Nov 2, 2024
25 checks passed
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.

3 participants