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

Convert show to three columns #1150

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Convert show to three columns #1150

merged 1 commit into from
Aug 12, 2024

Conversation

dnoneill
Copy link
Contributor

@dnoneill dnoneill commented Aug 9, 2024

Screenshot 2024-08-12 at 4 17 14 PM
Screenshot 2024-08-12 at 4 16 40 PM

@dnoneill dnoneill marked this pull request as ready for review August 9, 2024 16:13
@dnoneill dnoneill marked this pull request as draft August 9, 2024 16:19
@dnoneill dnoneill mentioned this pull request Aug 8, 2024
8 tasks
@dnoneill dnoneill marked this pull request as ready for review August 12, 2024 16:10
@dnoneill dnoneill changed the title fix show to match geoblacklight Convert show to three columns, add georeference alert Aug 12, 2024
@dnoneill dnoneill force-pushed the bl8-show branch 3 times, most recently from 9a8b5d3 to 4ca8de2 Compare August 12, 2024 19:13
Copy link
Member

@thatbudakguy thatbudakguy left a comment

Choose a reason for hiding this comment

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

Could we split the georeference alert off of this? Would be easier to review just the display change, and then the added alert can come after

Comment on lines 81 to 79
.help-text.viewer_protocol {
display: none;
}
Copy link
Member

Choose a reason for hiding this comment

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

I know it isn't in the designs but I'm slightly worried about hiding this, if only because it makes it way easier in testing to see what viewer is actually getting rendered/what the data is supposed to be. Maybe we need to check if @dbranchini hid it intentionally, and if not if there's a way to re-add it to the page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove it for now and we can add it back in if Darcy says it was purposeful

Choose a reason for hiding this comment

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

I'm not sure what this is referring to? What did I eliminate? It probably wasn't intentional.

<div class="bi bi-info-circle-fill fs-3 me-3 align-self-center
d-flex justify-content-center"></div>
<div class="text-body">
<div>This map is not georeferenced.</div>
Copy link
Member

Choose a reason for hiding this comment

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

I know we don't do it everywhere, but imo it's good practice to put these strings in the locale file, at least for new content/templates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the georeference alert. Making new branch.

@dnoneill dnoneill changed the title Convert show to three columns, add georeference alert Convert show to three columns Aug 12, 2024
@dnoneill dnoneill merged commit b4d12c7 into bl8 Aug 12, 2024
3 checks passed
@dnoneill dnoneill deleted the bl8-show branch August 12, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants