-
-
Notifications
You must be signed in to change notification settings - Fork 778
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
Update toolkit yml files to include a new fields #3431
Update toolkit yml files to include a new fields #3431
Conversation
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
|
Availability: 7/28-7/30 |
ETA:7/27 |
I seem to be having some issues with VS Code which will delay my review. |
Availability: 6 Hours |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reverts commit that fixes lint error by deleting the lines that caused errors.
Reverted the last commit. The branch should look normal now. I'll have to fix the lint errors, but don't know how without causing those weird formatting issue. |
Yup, the text formatting looks normal now. Can't say much on how to resolve the lint issues as I'm not too familiar with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Zak234 How do you test for other types (i.e. tablet, phone, etc)? |
I might have to ask the design team how they want the tablet "External Resources" title to be formatted because right now, it is the same as guides. The changes I made makes External Resources filterable by practice areas (Development, Design, etc). If there's only 1 resource, you get there undesirable formatting. |
I'm not sure exactly what is causing the issue. I think it may just be due to there only being one project card as opposed to two under the External Resources which messes with the spacing. I'm pretty sure this is the case as when put into mobile view, which only has one project card under External Resources, the HFLA site has the same weird formatting. |
Talked to Bonnie. The Guides and External Resources heading on tablet should not be centered. I will adjust the code for that. More additions to this issue will be discussed on Friday at Guides meeting and commented in #3357. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Tung!!
I sat down and looked at the linter issues and it looks like the linter is angry about some duplicative code in _sass/components/_toolkit.scss. Particularly .resource-svg-icons which is defined on line 83, 168, and 188. It is also upset about .toolkit-info-container defined on line 89, 101, and 193. The last one it is complaining about is h3 on lines 93 and 194.
I'll be MIA for the next 3 days, picked up some extra shifts. I'll add the additional functionalities and fix the lint issues on Monday. |
I've updated the first pull request comment to reflect the new changes. Pushed new commits, will fix lint issues after. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request comes from the correct branch and goes into the correct branch. It has a linked issue that the PR addresses. In all of the _guide-pages and the toolkitresources.yml: the category has been changed to practice area and the following fields have been created tools, contributors, source, recommended-by, changed status to work in progress/resource-url-wip/resource-url-completed. Changed Guide pages' status to work in progress/completed/depreciated.
In guid-card.html changed logic for displaying guide card links, Changed Guide pages with status of coming soon to work in progress. Added logic to display description only if it's less than 210 characters, otherwise it'll display short description.
In toolkit.html the external-resource-container was deleted and the cards are not centered.
In _toolkit.scss that container was also removed and changed to display 2 cards max and not be centered. I did notice in Nest Hub Max view that there were three but it still looked good. And the classes of .resource-svg-icon and .toolkit-info-container were consolidated.
The toolkit yml files have been updated to include new fields like the linked issue wanted and the Toolkit files template reflects those changes.
In the browser it looks good and previous issues have been resolved.
Good Job!!
ETA: End of Day on 8/31 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tunglinn Great work on this rather involved issue. All of your changes look good. The only issue I have is this stray line of code I'm seeing locally at the bottom of the toolkit page. If you remove that, we'll get it merged right away. Thanks for all your hard work on this one.
@kathrynsilvaconway I don't see this visual when I run the branch locally. I'll be at tonight's meeting to hopefully resolve this with you. |
ETA: EOD 2022-09-05 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tunglinn If you check in _pages/toolkit.html. Comment out first the Line 57, 58, 88, 120, 123, 152 and then delete them (coming from because the linter was angry earlier and has left effect. Will help to resolve the conflicts you're having as @kathrynsilvaconway mentioned. It will help to get the PR merged as you did such a great job and changes are correct!!
PS: you won't see these lines because linter won't allow to add them on GitHub, but they're when you open the text-editor and so do on website.
I fixed the merge conflict, which was due to a recent refactoring of the toolkit.html file. @arpitapandya I fixed the Lint issue before and the check is successful. Which Lint issue were you referring to? |
When I picked up this PR to review there was a merge conflict in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @tunglinn !
Dismissing this review in order to get this PR merged. All changes look good and issues have been addressed.
Fixes #3357
What changes did you make and why did you make them ?
In all the files in /_guide-pages and in the file /_data/internal/toolkitresources.yml
provider-link to resource-urlprovider-link is now three different fields depending on the state of the guide-page: resource-url-completed, resource-url-wip, resource-url-depreciatedshort-description,link-svg (this also required some changes in /pages/toolkit.html)In /_includes/guide-card.html
In /pages/toolkit.html
In /_sass/components/_toolkit.scss
Overall, changes were made to add/update card fields and make sure the website does not change unless specified.
For the field information, check this comment and others in the issue.
The wiki templates can be found under File Templates in the Wiki.
Purpose is to implement new design/features for the Toolkit page.
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Visuals after changes are applied