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

SHS-6015: Review and update humsci_basic theme readme.md #1718

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

mariannuar
Copy link
Collaborator

@mariannuar mariannuar commented Jan 15, 2025

Summary

Review and update humsci_basic readme.md

Need Review By (Date)

01/24

Urgency

low

Notes

I asked H&S is HumSci Airy is still being used, because if it's not, to remove it from the diagram and the text. Still waiting for an answer.

Steps to Test

  1. Confirm Diagram references Drupal 10
  2. Confirm Diagram and the read me text is not showing any related to the legacy theme which is now removed
  3. Branch names are updated
  4. Since we are still using decanter, we still need it in the diagram and the text

Review Tasks

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Are PHP functions and variables in snake_case and not camelCase?
  • Does Drupal code follow Drupal Coding 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?

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?

@ahughes3 ahughes3 temporarily deployed to Tugboat January 15, 2025 22:20 Destroyed
@mariannuar mariannuar self-assigned this Jan 15, 2025
@dalin- dalin- changed the title SHS-6015: Review and update humsci_basic readme.md SHS-6015: Review and update humsci_basic theme readme.md Jan 16, 2025
@mariannuar mariannuar removed the request for review from callinmullaney January 20, 2025 15:54
Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

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

@mariannuar This is a good start, however there are some things that can be improved.

  • Stanford Basic is still in the codebase, but not used anymore. It can be removed from the diagram
  • Let's wait on @ahughes3 answer about Airy usage
  • There are no site-specific themes, this can be removed from the diagram

The "Visual Regression Testing" section needs some improvements. For example, there's no clarity on the different systems available, the linting information is inside it even when it's not directly related to VRT, etc. Also, it would be a good idea to include some information on Tugboat Visual Diff (assuming we're using it). Add some general documentation on how can be used, how to add new pages, etc.

docroot/themes/humsci/humsci_basic/readme.md Outdated Show resolved Hide resolved
…n the different systems available, and move linting test to other section
@ahughes3 ahughes3 temporarily deployed to Tugboat January 21, 2025 15:28 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat January 21, 2025 16:20 Destroyed
@mariannuar
Copy link
Collaborator Author

@cienvaras Thank you! I just updated it the readme.md file and the diagram according to your feedback. I think the only thing missing is the answer about Airy usage. I just asked @ahughes3 in the Slack channel. I'll wait!

@mariannuar
Copy link
Collaborator Author

Ready for review @cienvaras

Base automatically changed from 11.6.3-release to develop January 22, 2025 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants