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

Tco80 simulator #187

Merged
merged 6 commits into from
Feb 20, 2025
Merged

Tco80 simulator #187

merged 6 commits into from
Feb 20, 2025

Conversation

JPrevost
Copy link
Member

@JPrevost JPrevost commented Feb 13, 2025

Summary of changes (please refer to commit messages for full details)

Adds a demo/simulation UI including interventions that could be used in other interfaces, such as bento or timdex-ui.

Developer

Ticket(s)

Accessibility

  • ANDI or Wave has been run in accordance to our guide and
    all issues introduced by these changes have been resolved or opened
    as new issues (link to those issues in the Pull Request details above)
  • There are no accessibility implications to this change

Documentation

  • Project documentation has been updated, and yard output previewed
  • No documentation changes are needed

ENV

  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.

Stakeholders

  • Stakeholder approval has been confirmed
  • Stakeholder approval is not needed

Dependencies and migrations

NO dependencies are updated

NO migrations are included

Reviewer

Code

  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.

Documentation

  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.

Testing

  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-187 February 13, 2025 22:07 Inactive
@JPrevost JPrevost temporarily deployed to tacos-api-pipeline-pr-187 February 14, 2025 18:36 Inactive
- `app/view/layouts/_head.html.erb` is taken from timdex-ui
- `app/javascript/controllers/content_loader_controller.js` is an example from stimulus docs
- `/app/javascript/controllers/index.js` switched to lazyLoad because it seemed to work whereas the default didn't
Why are these changes being introduced:

* We want a non-logging method to let people (and us) see what TACOS
  understands about any possible input

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/TCO-121
* https://mitlibraries.atlassian.net/browse/TCO-80

How does this address that need:

* Creates page that will display detector data for any given term

Document any side effects to this change:

* Includes some styles pulled from another app. Whether these should be
part of the style library is an open question.
Why are these changes being introduced:

* providing HTML interventions for tacos detected data from tacos
  itself rather than having each consuming application build them
  seems worth trying

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/TCO-115

How does this address that need:

* Creates a route that accepts a doi input and returns HTML without
  any layouts

Document any side effects to this change:

* The HTML markup assumes it will be embedded in a page with MITL styles
* This UI is a placeholder. UXWS will design the actual markup in
https://mitlibraries.atlassian.net/browse/TCO-144
@JPrevost JPrevost temporarily deployed to tacos-api-pipeline-pr-187 February 14, 2025 19:25 Inactive
@JPrevost JPrevost temporarily deployed to tacos-api-pipeline-pr-187 February 14, 2025 20:26 Inactive
@jazairi jazairi self-assigned this Feb 18, 2025
Copy link
Contributor

@jazairi jazairi left a comment

Choose a reason for hiding this comment

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

I agree that the flagged security issue isn't relevant for us, so I went ahead and dismissed it. My only comment is non-blocking (theme gem updates seem well out-of-scope for this).

Speaking of that, though, I wonder if the TIMDEX UI changes to _head.html.erb can be incorporated upstream, along with the SHAs. Looking back at the TIMDEX UI repo, the reason we didn't do it then is because we hadn't tested Turbo with non-Turbo apps. Not sure when we'll have bandwidth for theme gem maintenance, but it could be worth a look when the time comes.

I didn't spend too much time with the UI, since you noted that UXWS will design the final markup. In the meantime, everything seems to be working fine, and automatic a11y checks came through clear.

Nice work on this! I have a feeling the demo and intervention interfaces will help with the k-nn work.

@JPrevost JPrevost merged commit 6454407 into main Feb 20, 2025
6 checks passed
@JPrevost JPrevost deleted the tco80-simulator branch February 20, 2025 13:49
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.

3 participants