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 option to display URL below title #365

Merged
merged 5 commits into from
May 18, 2023

Conversation

bah0
Copy link
Contributor

@bah0 bah0 commented Nov 5, 2022

Hi! Thanks for your awesome project!

I needed a feature to display the URL of bookmark below the title in bookmarks list.
I added a setting in the general page and implemented the changes necessary in the bookmark list to display it.

Feature implementation as seen in screenshot below:

url_display

@adamshand
Copy link
Contributor

I'd find this very helpful as well, thanks!

@sebw
Copy link

sebw commented Dec 12, 2022

Related to a staled PR #219

@bah0
Copy link
Contributor Author

bah0 commented Dec 12, 2022

Thanks @sebw for the hint. I did not check the staled PRs.

I will review the ToDos in detail, but from short look at it, most of the tasks are implemented as requested. I have yet to verify / implement unit tests for this PR. Hoping it to be merged soon. :)

Copy link
Owner

@sissbruecker sissbruecker left a comment

Choose a reason for hiding this comment

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

Thanks, added a few comments. Tests should be added, probably to test_bookmarks_list_tag.py.

@@ -58,6 +58,10 @@ ul.bookmark-list {
text-overflow: ellipsis;
}

.url-display {
color: $green-color;
Copy link
Owner

Choose a reason for hiding this comment

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

Not happy with adding another color to the palette, the current green feels very "off". Maybe we could just lighten / darken the default link color somewhat.

From some testing:

  • Light theme: rgba(87, 85, 217, 0.64)
  • Dark theme: rgba(168, 177, 255, 0.73)

Copy link
Contributor Author

@bah0 bah0 Jan 22, 2023

Choose a reason for hiding this comment

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

Renamed variable to $secondary-link-color and added palette colors to respective scss files for dark and light themes. Waiting for other tasks before pushing changes.

Comment on lines 27 to 28
$green-color: #8aaa7c;
$green-color-dark: #4a912b;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
$green-color: #8aaa7c;
$green-color-dark: #4a912b;
$secondary-link-color: #8aaa7c;

$green-color-dark doesn't seem to be used. Since $green-color is just another variant of $link-color, maybe rename it to $secondary-link-color.

Let's also move this up to the other link color variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did changes as requested, doing other tasks before pushing changes.

@@ -14,6 +14,11 @@
{{ bookmark.resolved_title }}
</a>
</div>
{% if request.user.profile.display_url %}
<div class="url-path truncate">
<span class="url-display text-sm">{{ bookmark.url }}</span>
Copy link
Owner

Choose a reason for hiding this comment

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

This should use an anchor tag so you can actually click on it. Can reuse the one from the title, minus the unread / italic styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -60,6 +60,15 @@ <h2>Profile</h2>
Disabling this feature will hide all previously shared bookmarks from other users.
</div>
</div>
<div class="form-group">
Copy link
Owner

Choose a reason for hiding this comment

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

Let's move this up to maybe below "Bookmark date format" , so that bookmark display options are somewhat grouped together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did changes as requested, doing other changes before pushing updates

@@ -1,12 +1,12 @@
{
"name": "linkding",
"version": "1.11.1",
Copy link
Owner

Choose a reason for hiding this comment

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

Let's revert changes in this file. It should probably be updated, but not in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted change

<div class="form-group">
<label for="{{ form.display_url.id_for_label }}" class="form-checkbox">
{{ form.display_url }}
<i class="form-icon"></i> Display URL below title
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
<i class="form-icon"></i> Display URL below title
<i class="form-icon"></i> Show bookmark URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

<i class="form-icon"></i> Display URL below title
</label>
<div class="form-input-hint">
When enabled, this setting displays the URL of the entry below the title.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
When enabled, this setting displays the URL of the entry below the title.
When enabled, this setting displays the bookmark URL below the title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@M4NH3X
Copy link

M4NH3X commented Mar 1, 2023

This feature looks good aside the initial design. Thanks @bah0 for working on it.🍺

@sissbruecker
Copy link
Owner

Thanks for addressing the comments, I rebased and cleaned up the migrations a bit.

@sissbruecker sissbruecker changed the title Add feature to display URL below title Add option to display URL below title May 18, 2023
@sissbruecker sissbruecker merged commit bc374e9 into sissbruecker:master May 18, 2023
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.

5 participants