Skip to content

Update sites-filter.js#62

Merged
carlj11 merged 9 commits into
mainfrom
carlj_update_exhibit_display_order_and_favicon
Jul 14, 2025
Merged

Update sites-filter.js#62
carlj11 merged 9 commits into
mainfrom
carlj_update_exhibit_display_order_and_favicon

Conversation

@carlj11
Copy link
Copy Markdown
Contributor

@carlj11 carlj11 commented Jun 25, 2025

Add condition &sort_by=created&sort_order=desc (L100) to display most recent exhibit first on the exhibit landing page. This change is for the item_set sort oder but let's see if this actually changes the Site sort order on the main home page. Default behavior is to display the oldest (by date) created Site first.

Developer

Test setting exhibit Site sort order by created&sort_order=desc date to render most recently created exhibit Sites first.

orig:

  • let url = '/api/items?item_set_id=${id}'

now:

  • let url = '/api/items?item_set_id=${id}&sort_by=created&sort_order=desc'

Write a brief summary of changes here, but do not repeat commit messages (which
should have a more complete description).

Tickets affected

Version (see config/theme.ini)

  • [ X] The theme's version number has been incremented, setting up a production
    deploy.

Version number has now been updated to 1.0.1. NOTE: at this point we're not ready for a production deploy but this may help with debugging some issues with the automated update process.

Documentation

  • Project documentation has been updated.
  • No documentation changes are needed.

Accessibility

  • ANDI or WAVE has been run in accordance to our guide.
  • New flagged issues not already resolved in this PR have been ticketed
    (link in the Pull Request details above).
  • This PR contains no changes to the view layer.

Stakeholder approval

  • Stakeholder approval has been confirmed (see ticket).
  • Stakeholder approval will happen retroactively.
  • Stakeholder approval is not needed.

Dependencies

  • New dependencies have been added
  • Dependencies have been updated
  • No dependencies have changed

Additional context needed to review

E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.

NOTE: 6/27 I tried running the 'make update' step (didn't realize I could run prior to submitting this PR) which is supposed to update the favicon but that failed. See our JIRA ticket: https://mitlibraries.atlassian.net/browse/IN-1327

Code Reviewer

Code
  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.
  • New dependencies are appropriate or there were no changes.
Documentation
  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.

Add condition &sort_by=created&sort_order=desc (L100) to display most recent exhibit first on the exhibit landing page. This change is for the item_set sort oder but let's see if this actually changes the Site sort order on the main home page. Default behavior is to display the oldest (by date) created Site first.
@matt-bernhardt matt-bernhardt self-assigned this Jun 25, 2025
@matt-bernhardt
Copy link
Copy Markdown
Member

I've run the workflow to deploy this branch to staging, and am trying to confirm that things work as expected. The review is under way, and I'm hoping to have something meaningful to say tomorrow.

Deploy workflow result: https://github.com/MITLibraries/mitlibraries-theme-omeka/actions/runs/15887300608/job/44802306845

@matt-bernhardt
Copy link
Copy Markdown
Member

I'm not seeing an update to the deployed sites-filter.js file, like I'd expect to see during a deploy. Compare the deployed file:

https://mitlibraries-stage.omeka.net/themes/mitlibraries-theme-omeka/asset/js/sites-filter.js?v=1.0

with the version in this PR:

https://github.com/MITLibraries/mitlibraries-theme-omeka/pull/62/files#diff-a077944fb4f41f41929c706212f6703837423cea26c34f6ce37b940104ed2f04

My expectation was that the change near the bottom of the file would be reflected, but I don't see it yet.

As a second check, Carl could you please try bumping the version number of the theme by incrementing this line from 1.0 to 1.0.1? That would give us something else to look at that would be an outcome of the deploy process.

https://github.com/MITLibraries/mitlibraries-theme-omeka/blob/main/config/theme.ini#L3

@matt-bernhardt
Copy link
Copy Markdown
Member

On a related note, I don't see in this PR a change to the favicon yet - has this been added, or is that still on the to-do list? Running make update from the theme root should result in a change to asset/img/mitlib-style/favicon.ico

carlj11 and others added 2 commits June 27, 2025 02:20
(carlj, 2025-06-27) After Matt B. consult, add small snippet of styles to remove unwanted horizontal grey bar. Necessitated by updated Block behavior by omeka.org/DS in Omeka version 4.1.1.

DS says the rule in. question is:

.page-layout-normal .block {
   margin: 1rem 0;
}
Increment version number to 1.0.1
@carlj11
Copy link
Copy Markdown
Contributor Author

carlj11 commented Jun 27, 2025

Incremented version number to 1.0.1 in config/theme.ini.

I just tried running 'make update' from my local branch and it failed with an error in the end:

cp: tmp/mitlib-style-master/_assets/i/mitlib-wordmark.svg: No such file or directory

carlj11 added 5 commits June 27, 2025 11:16
Add snippet of styles to remove unwanted horizontal grey bar. Make everything inside the .wrap-page appear on a white background  to prevent the body background color from bleeding through
Indent snippet to make everything inside .wrap-page appear on a white background.
Copy link
Copy Markdown
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

This is looking good to me in the abstract, but I'm going to hold off on approving it formally until we get it deployed to staging to confirm that everything looks okay in action.

…n in package.json; package-lock.json integrity sha512- value update(?)

Remove verbose reference to the date of my changes to the file in _layouts.scss

We don't fully know what the changes to pakcage-lock.json and package.json mean. Changes to integrity sha512-  and stylelint version package.json. Matt B. didn't think whatever warranted the commit was significant but to go ahead and commit anyway. We didn't ourselves touch the files.
Copy link
Copy Markdown
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

This all looks good to me, and when I deploy it to the staging tier it seems to work as expected.

We'll need a follow-up ticket to update the theme to load some assets from our CDN rather than package them with the theme itself (which should also include some markup changes to how the favicon gets loaded, which I've described in ES-3204) - but for now, it seems like Stephanie is okay with what's here.

I'm comfortable with merging this as-is, but if you have concerns about the sort order you mentioned in the ticket, we can talk through those before merging.

@carlj11 carlj11 merged commit 3bb2750 into main Jul 14, 2025
8 checks passed
@carlj11 carlj11 deleted the carlj_update_exhibit_display_order_and_favicon branch July 14, 2025 22:18
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.

2 participants