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

Fix format and record-versions commas #2002

Merged
merged 2 commits into from Jul 21, 2021

Conversation

crhallberg
Copy link
Contributor

@crhallberg crhallberg commented Jun 23, 2021

Fix these commas, which come from CSS:

image

TODO:

  • Run lessToSass when styles are finalized.

@demiankatz
Copy link
Member

Thanks, @crhallberg -- just looks like this needs a lessToSass in order to pass the build.

Also, exactly which case is .record .format:not(.badge)::after used for? I assume the screen shot above is what is fixed by .record-versions .format::after, but I wasn't sure what the other one was doing.

@crhallberg
Copy link
Contributor Author

The :not(.badge) is an additional case that would cover undiscovered combinations that would result in the same erroneous commas.

@demiankatz
Copy link
Member

@crhallberg, when I test this branch on my development box, I'm still seeing unwanted commas inside badges inside the Versions tab of the record view:

image

I'm still unclear about the :not(.badge) scenario, though -- this seems to be saying "do not display commas on the .format class if it is not a .badge" but that seems to be the opposite of what we want. Don't we want to omit commas when it IS a badge?

@demiankatz demiankatz added this to the 8.0 milestone Jul 8, 2021
@crhallberg
Copy link
Contributor Author

Oops! I definitely mixed up my cases and made the selector the opposite of what it should be. Can you give this update a shot?

@demiankatz demiankatz force-pushed the format-comma-fixes branch 2 times, most recently from 94af045 to 835b081 Compare July 21, 2021 11:55
@demiankatz demiankatz changed the base branch from dev to release-7.1 July 21, 2021 11:55
@demiankatz
Copy link
Member

Thanks for the adjustments, @crhallberg. It makes more sense now, but it still wasn't quite right. I moved these changes over to a copy of the release-7.1 branch and changed the base of this PR, since this is a bug fix that we probably want sooner rather than later. I then added 835b081, which adds a line to fix the real cause of the unwanted commas in the versions tab. I'm now uncertain whether or not the previous line that you added is actually necessary. Any thoughts? I can do some more testing and adjustment as needed, but wanted to get your opinion before investing more time.

@demiankatz demiankatz merged commit a203a1a into vufind-org:release-7.1 Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants