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

feat: added extensions tab #15

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

Conversation

Majortheus
Copy link
Contributor

@Majortheus Majortheus commented Feb 22, 2023

When I was first visiting the web site, I wanted to see what extensions @diego3g was using without to search manually or changing my "extensions.json", so again I tried to implement what i thot was the most interesting approach, which was to adapt the "extensions.json" to create a extensions tab similar to the code, I didn't go deeper on the clone as I thot some items whore unnecessary , to challenge myself and to achieve a result I was proud about. Comments and suggestions are very much welcome 😄, especially because there are some struct/folder changes to use and abuse the new layout systems of NextJS 13.

Preview:
preview

BTW, Catppuccin extensions seems like a cool theme, and Polacode a interesting extension, this was one of the reasons I did this PR, to have a peek on what could work for me...

@vercel
Copy link

vercel bot commented Feb 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
fala-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 12, 2023 at 4:27AM (UTC)

@diego3g
Copy link
Owner

diego3g commented Feb 22, 2023

That's awesome! I will review this with my points pretty soon! I recently created an issue (#17) to remember this so it's cool that we have an implementation start point!

@diego3g diego3g self-requested a review February 22, 2023 13:23
@diego3g diego3g added the enhancement New feature or request label Feb 22, 2023
@b2evandro
Copy link

image

Copy link
Owner

@diego3g diego3g left a comment

Choose a reason for hiding this comment

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

First, great job!

  1. By creating a new explorer folder inside app, the URL not have a explorer prefix on all pages. Maybe we could rename it to (explorer) so it will not affect the URL, what do you think?
  2. Instead of the blank page in explorer/page.tsx, maybe we could have a 404.tsx;

import "src/styles/extensions-markdown.css";

export const metadata = {
title: "Extenções",
Copy link
Owner

Choose a reason for hiding this comment

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

Can we use generateMetadata function to add the ID of the extension in the title as well? https://beta.nextjs.org/docs/guides/seo#dynamic-metadata

}
);

const response: ExtensionsAPIResponseType = await data.json();
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure, but I think this code is also valid:

Suggested change
const response: ExtensionsAPIResponseType = await data.json();
const response = await data.json<ExtensionsAPIResponseType>();

}: {
params: { extension: string };
}) {
const data = await fetch(
Copy link
Owner

Choose a reason for hiding this comment

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

This could be less generic, maybe extensionDetailsResponse.

And, for the data, we could use something like:

const extensionData = await extensionDetailsResponse.json()

Comment on lines 30 to 34
criteria: [{ filterType: 4, value: params.extension }],
},
],
assetTypes: [],
flags: 950,
Copy link
Owner

Choose a reason for hiding this comment

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

filterType and flags here are magic numbers, I mean, it's not clear what they mean...

Can you create a variable on the top of this file with a constant for each one describing what they mean?

Something like:

const MARKETPLACE_FILTER_GET_EXTENSION_DESCRIPTION = 4;

Comment on lines 41 to 43
if (!response || !response.results || !response.results[0].extensions || !response.results[0].extensions[0]) {
return <div></div>;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of returning an empty div here, we could throw an error and create an global error.tsx page inside app/error.tsx.

Comment on lines +63 to +67
extension.versions[0].files.find(
(file) =>
file.assetType ===
"Microsoft.VisualStudio.Services.Icons.Default"
)?.source ?? ""
Copy link
Owner

Choose a reason for hiding this comment

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

This could be a variable above return.

<div className="mt-2">
<Link
target="_blank"
href={`https://marketplace.visualstudio.com/items?itemName=${extension.publisher.publisherName}.${extension.extensionName}`}
Copy link
Owner

Choose a reason for hiding this comment

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

As we are already displaying all the information about the extension inside this page, I think this link could redirect the user to the install itself, instead of redirecting to the marketplace URL.

className="min-w-[42px] h-[42px]"
/>
</div>
<div className="flex flex-col leading-5 text-sm text-[#908caa] max-w-full w-[calc(256px-42px-16px-24px)]"> {/* calc = 100% parent width (256) - img width (42) - parent padding left (16) - padding from scroll (24) */}
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this calc is necessary while using flex or grid 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is with the calc
image

This is without
image

basically the div element doesn't respect the parents width, and text oveflow "ellipsis" will not show because its hidden behind the content panel, if you have suggestions on how to make this div respect it's parent width, I will happily make the change, I also didn't like the calc as it's confusing, but it was the only way to mimic visual code style, and to be a little bit less confusing I added the comment

<Icon
strokeWidth={1.5}
size={28}
className="text-[#8F8CA8] data-[active=true]:text-[#E0DEF2]"
Copy link
Owner

Choose a reason for hiding this comment

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

Did this work? I think you need to set a group class in parent div and use group-data-[active=true] here.

</div>
</div>
<div className="max-h-full text-sm mt-8 extensions-markdown">
<ReactMarkdown rehypePlugins={[rehypeRaw]}>{detailsText}</ReactMarkdown>
Copy link
Owner

Choose a reason for hiding this comment

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

What are react-markdown and rehype responsibilities here? I mean, what each one is doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The detailsText it's in markdown format by default, and I thot it was easy to have a lib to handle that, but when I was testing the results, I came across some extensions that have html inside it's markdowns, and rehybe plugins handle that rendering inside the markdown, otherwise the text was rendering as text and I couldn't use css styles to make exactly like visual studio

@Majortheus
Copy link
Contributor Author

Majortheus commented Mar 10, 2023

OMG, just saw the newest Rocketseat video, completely forgot about this PR, sorry... it has been a really busy couple of weeks, I will take a look real soon

@dilincoln
Copy link

dilincoln commented Mar 10, 2023

Hi everyone, I was testing this beautiful feature and I faced this issue below:

CleanShot 2023-03-10 at 14 43 48

@Majortheus
Copy link
Contributor Author

Changes made, I think it didn't change much, some changes was to make (explorer) work, because it was hard to think a way to make any URL work as active except the ones that are already in use in menu, but I found a way, not sure if it's the best one, but it was interesting to make work.

Did some refactoring as was pointed out, way better naming, simplified active icon on menu, as was not working correctly, fixed bug pointed by @dilincoln, not sure why was happening, but a new deploy seems to work.

It's still bothering me the fact that tabs is working in localhost, but not on deployed version, and I still have no clue on why that's happening.

P.S.: I was thinking that can be fun to make the others tabs work, like:

  • Search: make a full search on site and shows a link with maybe #:~:text= to highlight the text on web page, the git showing
  • Git/Source Control: Show what PRs was accepted on this repository, or what changes was made to file texts or what extension went from active to unactive, or vice versa, like a history log, for instance: on May 13 @diego3g changed his default icons to material icons, or something like that
  • Debug: I don't have many ideas on what to do with this tab, I was thinking some along the lines of showing the "code" behind the page
  • Remote Explorer: I think that would be awesome if @diego3g could make other Rocketseat educator like Mayk Brito and Rodrigo Gonçalves and any other Rocketseat Educator that has shown their visual code to make their fork and fill with their information, and make this tab show their website as "remote workspaces" or something along this lines to show others "fala devs"
  • Experiments: I don't have this tab on my visual code, I don't know what it usually show, but maybe could be a suggestion box?

I think it would be incredible but extremely hard to pull off, and most of the ideas I don't know how exactly I would do it, so I would love to hear what you guys think, and if it is realistic to expend that much time on this project that basically has done what was build for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants