www
/
www-new
16
3
Fork 7
Code Issues 58 Pull Requests 47 Projects Releases Wiki Activity

internship-guide #649

Closed
ry3zhu wants to merge 8 commits from internship-guide into main
pull from: internship-guide
merge into: www:main
Conversation 0 Commits 8 Files Changed 13 +541 -2
ry3zhu commented 2024-01-28 23:17:27 -05:00
Member
Copy Link
No description provided.
ry3zhu added 4 commits 2024-01-28 23:17:29 -05:00
continuous-integration/drone/push Build is passing Details
8c43bc99fc Added Ryan Zhu to Webcom
continuous-integration/drone/push Build is failing Details
695d57438e First draft of internships page
continuous-integration/drone/push Build is failing Details
0445f3aabc Fix layout and styling on internships page
ry3zhu requested review from dlslo 2024-01-28 23:17:39 -05:00
dlslo requested changes 2024-02-05 23:25:32 -05:00
dlslo left a comment
Member
Copy Link

Great work, the page looks great! Just a few changes, mainly about cleaning up the code a little.

Great work, the page looks great! Just a few changes, mainly about cleaning up the code a little.
components/Navbar.tsx Outdated
@ -102,0 +112,4 @@
route: "/resources/internships/resources",
},
],
route: "/resources/internships/resume",
dlslo commented 2024-02-05 22:51:22 -05:00
Member
Copy Link

These links don't work. I think you will need to do something similar to the file/folder structure of advice to get it to work since NextJS creates its routes based on the project structure.

These links don't work. I think you will need to do something similar to the file/folder structure of `advice` to get it to work since NextJS creates its routes based on the project structure.
pages/resources/internships/index.module.css Outdated
@ -0,0 +40,4 @@
justify-content: space-between;
align-items: center;
border-bottom: 1px solid var(--light--author-text);
width: calc(50.52vw + 124.3px);
dlslo commented 2024-02-05 23:18:19 -05:00
Member
Copy Link

Imo its fine if the container widths are 50rem like in other pages. Make sure to use max-width instead of width for smaller screens.

Imo its fine if the container widths are 50rem like in other pages. Make sure to use max-width instead of width for smaller screens.
pages/resources/internships/index.module.css Outdated
@ -0,0 +56,4 @@
border-top-right-radius: calc(10rem / 16);
background-color: transparent;
color: var(--text);
font-size: calc(0.7vw + 7.566px);
dlslo commented 2024-02-05 23:22:39 -05:00
Member
Copy Link

We shouldn't have these precisely hardcoded values. Using 24px or something is fine and then scaling on smaller screens would be better.

We shouldn't have these precisely hardcoded values. Using 24px or something is fine and then scaling on smaller screens would be better.
pages/resources/internships/index.module.css Outdated
@ -0,0 +76,4 @@
.subpagesWrapper {
overflow-x: hidden;
margin: 0 calc(0.1924 * 100vw);
dlslo commented 2024-02-05 23:23:50 -05:00
Member
Copy Link

Using margin: auto here should work as long as you set a max-width: 50rem

Using `margin: auto` here should work as long as you set a `max-width: 50rem`
pages/resources/internships/index.module.css Outdated
@ -0,0 +84,4 @@
grid-template-columns: repeat(3, minmax(0, 1fr));
grid-template-rows: repeat(1, minmax(0, 1fr));
width: calc(3 * (50.52vw + 124.3px));
transition: transform 0.6s cubic-bezier(0.88, 0.1, 0.64, 0.95);
dlslo commented 2024-02-05 23:00:43 -05:00
Member
Copy Link

I think we can reduce this to ~0.2s

I think we can reduce this to ~0.2s
pages/resources/internships/index.module.css Outdated
@ -0,0 +100,4 @@
padding: 10px;
margin: 0 10px;
overflow-y: hidden;
transition: all 6s cubic-bezier(0.95, 0.34, 0, 1);
dlslo commented 2024-02-05 23:01:40 -05:00
Member
Copy Link

I think it's fine to remove this animation (the text appearing slowly). Removing it would also solve a bug of the old text remaining for a few seconds when switching between tabs

I think it's fine to remove this animation (the text appearing slowly). Removing it would also solve a bug of the old text remaining for a few seconds when switching between tabs
pages/resources/internships/index.module.css Outdated
@ -0,0 +110,4 @@
align-items: flex-start;
}
@media only screen and (max-width: calc(768rem / 16)) {
dlslo commented 2024-02-05 23:03:44 -05:00
Member
Copy Link

When in mobile, you will need to stack the tabs vertically rather than horizontally similar to in the advice page

When in mobile, you will need to stack the tabs vertically rather than horizontally similar to in the advice page
pages/resources/internships/index.tsx Outdated
@ -0,0 +31,4 @@
<Header />
</div>
<div className={styles.selectorContainer}>
{["Resume", "Interview", "Resources"].map((name, i) => (
dlslo commented 2024-02-05 22:59:57 -05:00
Member
Copy Link

You can use the subPagesConfig map here too and user interpolated strings.

You can use the subPagesConfig map here too and user interpolated strings.
pages/resources/internships/index.tsx Outdated
@ -0,0 +50,4 @@
<div className={styles.subpagesWrapper}>
<div
className={
styles.subpagesContainer +
dlslo commented 2024-02-05 22:56:22 -05:00
Member
Copy Link

Try using interpolated strings here. e.g.

const x = 3
const y= `hello ${x}` // hello 3
Try using interpolated strings here. e.g. ``` const x = 3 const y= `hello ${x}` // hello 3 ```
pages/resources/internships/index.tsx Outdated
@ -0,0 +56,4 @@
? styles.subpage0
: currentSubpage === 1
? styles.subpage1
: styles.subpage2)
dlslo commented 2024-02-05 22:58:45 -05:00
Member
Copy Link

You can reduce a lot of code duplication by having something like:

const subPagesConfig = [
    { heading: "Resume", component: <Resume />, style: styles.subpage0 },
    { heading: "Interview", component: <Interview />, style: styles.subpage1 },
    { heading: "Resource", component: <Resources />, style: styles.subpage2 },
  ];

Then you can map these pages:

{subPagesConfig.map(({ heading }, i) => (
          <button
            className={`${styles.selector} ${
              currentSubpage === i ? styles.selected : ""
            }`}
            key={heading}
            onClick={() => {
              setCurrentSubpage(i);
            }}
          >
            {heading}
          </button>
        ))}
You can reduce a lot of code duplication by having something like: ``` const subPagesConfig = [ { heading: "Resume", component: <Resume />, style: styles.subpage0 }, { heading: "Interview", component: <Interview />, style: styles.subpage1 }, { heading: "Resource", component: <Resources />, style: styles.subpage2 }, ]; ``` Then you can map these pages: ``` {subPagesConfig.map(({ heading }, i) => ( <button className={`${styles.selector} ${ currentSubpage === i ? styles.selected : "" }`} key={heading} onClick={() => { setCurrentSubpage(i); }} > {heading} </button> ))} ```
ry3zhu added 2 commits 2024-02-11 19:22:31 -05:00
ry3zhu added 1 commit 2024-02-19 16:39:13 -05:00
continuous-integration/drone/push Build is failing Details
0b00dc5d7f Redesign internships pages
ry3zhu added 1 commit 2024-02-19 16:51:30 -05:00
continuous-integration/drone/push Build is failing Details
267af98929 linting
a3thakra reviewed 2024-02-20 12:52:43 -05:00
components/Navbar.tsx
@ -102,0 +112,4 @@
route: "/resources/internships/resources",
},
],
route: "/resources/internships",
a3thakra commented 2024-02-20 12:52:43 -05:00
Owner
Copy Link

No need to do a redirect if we can avoid it, can you link it directly to the correct route?

No need to do a redirect if we can avoid it, can you link it directly to the correct route?
a3thakra reviewed 2024-02-20 13:00:11 -05:00
content/resources/internships/index.mdx
@ -0,0 +1,9 @@
import { Bubble } from "@/components/Bubble";
a3thakra commented 2024-02-20 13:00:11 -05:00
Owner
Copy Link

Please use regular markdown files where you can. In this case, the bubble react component simply encapsulates the entirety of the content. Instead of making this an MDX, you can make it a regular markdown file with just the content (.md) and then encapsulate the content with the bubble component in the .tsx file.

This helps us keep our content and implementation separate, which will be helpful if we want to customize things in the future.

Please use regular markdown files where you can. In this case, the bubble react component simply encapsulates the entirety of the content. Instead of making this an MDX, you can make it a regular markdown file with just the content (.md) and then encapsulate the content with the bubble component in the .tsx file. This helps us keep our content and implementation separate, which will be helpful if we want to customize things in the future.
ry3zhu requested review from dlslo 2024-03-06 17:46:28 -05:00
ry3zhu closed this pull request 2024-03-07 21:04:37 -05:00
Some checks failed
continuous-integration/drone/push Build is failing
Required
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
dlslo
Labels
Clear labels   
a11y

   
Backlog

   
Blocked

   
Bug

   
Content

   
Dependencies

   
Design

   
Feature Request

   
Good First Issue

   
In Progress

   
Performance

   
Priority - High

   
Priority - Low

   
Priority - Medium

   
Untriaged
No Label
a11y
Backlog
Blocked
Bug
Content
Dependencies
Design
Feature Request
Good First Issue
In Progress
Performance
Priority - High
Priority - Low
Priority - Medium
Untriaged
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#649
No description provided.
Delete Branch "internship-guide"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?