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

Tweak add tag button and the look of tags. #3530

Merged
merged 3 commits into from Mar 21, 2024

Conversation

EreMaijala
Copy link
Contributor

  • Removes btn-link class that's meant for buttons and has unintended side-effects with Bootstrap 5.
  • Moves float from pull-right class to the .tag-record style because pull-right doesn't work the same in BS5.
  • Aligns the Add Tag button position, size and border with the tags. Tags now always have a border that ties the badge with the text better.

@demiankatz
Copy link
Member

@sturkel89, can you take a look at how this compares against dev in all three themes? This should just apply to the "add tag" controls on the record page.

@sturkel89
Copy link
Contributor

I'll document test vs. dev in all three themes in comments, and then I'll submit a review.

SANDAL

Dev version:
image

image

Test version:
image

image

@sturkel89
Copy link
Contributor

Bootprint3:

Dev version:
image

image

Test version:
image

image

@sturkel89
Copy link
Contributor

Bootstrap3:

Dev version:
image

image

Test version:
image

image

Copy link
Contributor

@sturkel89 sturkel89 left a comment

Choose a reason for hiding this comment

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

I like the new version. The outlines are crisp, and the visual change on click is much clearer to the eye (especially in sandal and bootstrap3). Good work on these updates!

Slight styling issues that may or not be worth adjusting:

Bootstrap3 - padding around button
I kept the browser width constant for test vs. dev in each theme (although I may have varied browser width between theme changes). This makes it clear that in bootstrap3, there's more padding around the new buttons, so the text next to the button scrolls to a fourth line. Maybe it would be better to compromise on the size of that padding?

Sandal - spacing in Tags row
In sandal, I always notice that the text in the Tags row has a much wider spacing than the text in the other rows in record view. (This is true in test and dev.) Can this be adjusted?? I know it's out of scope for this PR.

image

@EreMaijala
Copy link
Contributor Author

@sturkel89 I tweaked the padding and margin a bit on narrow displays to make the text fit on three lnes. And fixed the sandal line height issue as well while at it.

@demiankatz
Copy link
Member

Thanks, @EreMaijala and @sturkel89. I noticed there were some minor merge conflicts, and I just fixed them so that if @sturkel89 approves after the next round of testing, this will be ready to merge as-is.

Copy link
Contributor

@sturkel89 sturkel89 left a comment

Choose a reason for hiding this comment

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

This latest update fixes the two problems I identified. Very happy to see normal spacing in the Tags row in Sandal! I'll approve this for merging, and make an note of something that you may or may not want to deal with in another PR or Jira ticket.

I now notice that Sandal has two modes for wrapping in this the Tags row, probably related to a setting having to do with the record options at the top of the page.

  • At most browser widths, the text next to the Add Tag button scrolls to two lines (and the Cite this, Text this, etc. buttons are in one row).
  • Then, as you narrow, at one point the text next to Add Tag jumps to four rows -- no in-between!

I think this is because the Save to List and Permanent Link buttons at the top scroll around together to a second row, rather than Permanent Link moving to a second row on its own.

image

image

(All of this behavior is somewhat different in the other themes, just to keep things interesting!)

@demiankatz demiankatz removed the request for review from crhallberg March 21, 2024 14:11
@demiankatz demiankatz merged commit 1e0c905 into vufind-org:dev Mar 21, 2024
7 checks passed
@demiankatz demiankatz deleted the dev-tweak-add-tag-button branch March 21, 2024 14:11
@demiankatz demiankatz added this to the 10.0 milestone Mar 21, 2024
@EreMaijala
Copy link
Contributor Author

@sturkel89 I believe the 4-line wrap happens because the column is so narrow, and the font is larger than in other themes. I suppose it might make sense to make the heading column a bit narrower since it tends to have a lot of empty space. Not sure if there are cases where the space is actually needed, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants