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

Relayout of Training/Workshops and Service Catalog #23

Merged
merged 60 commits into from
Feb 13, 2024

Conversation

redndahead
Copy link
Contributor

READY FOR REVIEW

Summary

Review By (Date)

  • When does this need to be reviewed by? Whenever is good for you

Criticality

  • How critical is this PR on a 1-10 scale? 5

Urgency

  • How urgent is this? (Normal, High) Normal

Review Tasks

  • See above

Setup tasks and/or behavior to test

None

Site Configuration Sync

  • Is there a config:export in this PR that changes the config sync directory? None

Front End Validation

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided? eg (unit, behat, or codeception)

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

Affected Projects or Products

  • Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

  • JIRA ticket(s)
  • Other PRs
  • Any other contextual information that might be helpful (e.g., description of a bug that this PR fixes, new functionality that it adds, etc.)
  • Anyone who should be notified? (@mention them here)

Resources

redndahead and others added 30 commits November 1, 2023 14:00
Initial change to have a script create default content.
RHW-37: Make the SnapLogic URL Configurable
RHW-37: Add support for accessing main results using $ in token. Conv…
…s. Update service catalog to support the search and new filter handling.
RHW 40: Provided support for search in airtable lists.
RHW-45: Allow predefined values through the filter. Also update the url as the filters change.
RHW-42: Add a single page for all the training.
@@ -358,4 +502,19 @@
$('#airtable-list').height(newTop);
}

// debounce so filtering doesn't happen every millisecond
function debounce( fn, threshold ) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ehh Good point. This is probably a future improvement.

@sherakama
Copy link
Member

@redndahead I see a bunch of A11y stuff in here so thank you for that but a quick question for you. Has this functionality gone through a SODA review?

@redndahead
Copy link
Contributor Author

@redndahead I see a bunch of A11y stuff in here so thank you for that but a quick question for you. Has this functionality gone through a SODA review?

No we have not done that.

@sherakama
Copy link
Member

@redndahead I see a bunch of A11y stuff in here so thank you for that but a quick question for you. Has this functionality gone through a SODA review?

No we have not done that.

Ok, I'm not going to make that a requirement for this for approval or anything but with the interactivity it would be good to get their eyes on this. We can request a review once there is a live URL for them.

@sherakama
Copy link
Member

I'll continue to review this shortly.

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

Approving.

@jenbreese do you want to take a pass as well?

Also, I'd like to get this submitted to SODA for review. Once it is up on the test site, let's submit a ticket for them to have a pass on the service-catalog page.

@jenbreese
Copy link
Contributor

@redndahead I think it looks good. I noticed you have commented out some css and left it in. I would suggest removing it since it will be in the repo anyway if you need it later.

@redndahead
Copy link
Contributor Author

I have another PR I need to send over. Since this has already been reviewed would you like to merge this and then my next PR will only have a couple of changes?

@sherakama
Copy link
Member

sherakama commented Feb 5, 2024

I have another PR I need to send over. Since this has already been reviewed would you like to merge this and then my next PR will only have a couple of changes?

Smaller PRs are preferred. You can merge this and open another one or PR stack on top of this.

@redndahead
Copy link
Contributor Author

@sherakama @jenbreese I can't merge this PR as I don't have the permissions. Can you merge it and then I'll send over another PR.

@buttonwillowsix buttonwillowsix self-assigned this Feb 6, 2024
@buttonwillowsix
Copy link
Contributor

@redndahead if you want to merge this, you can resolve the conflicts and I can take care of it!

@buttonwillowsix buttonwillowsix removed the request for review from jenbreese February 6, 2024 04:30
@redndahead
Copy link
Contributor Author

@sherakama @jenbreese I have resolved the conflicts. Please merge and deploy as soon as possible please.

@sherakama sherakama merged commit 717fadd into SU-SWS:main Feb 13, 2024
1 check failed
@sherakama
Copy link
Member

Thanks @redndahead going ahead with the merge.

Do we have a test environment or does this get deployed straight to production?

@cdchangr
Copy link
Contributor

Hi @sherakama,
@redndahead is out today, so I am jumping in. Our test site is: https://researchhubsubtheme.sites.stanford.edu/
Thanks very much!

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.

5 participants