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

refactor: replace tabs with arrows [IDE-294] #540

Merged
merged 10 commits into from
Jun 12, 2024

Conversation

cat2608
Copy link
Contributor

@cat2608 cat2608 commented Jun 10, 2024

Description

This PR refactors the layout of fixed code examples by replacing the tab-based navigation with arrow-based navigation. It also includes several styling and script improvements.

  • Removed the Community Examples tab layout.
  • Implemented arrow-based navigation for fixed code examples.
  • Reorganised constants, variables, and functions.
  • Moved DOM-dependent functions inside DOMContentLoaded.

Checklist

  • Tests added and all succeed
  • Linted
  • README.md updated, if user-facing
  • License file updated, if new 3rd-party dependency is introduced

🚨After having merged, please update the CLI go.mod to pull in latest language server.

Screenshots / GIFs

IntelliJ VSCode
image image

@cat2608 cat2608 force-pushed the refactor/IDE-294-replace-tabs-with-arrows branch from 48fa526 to aa4fd53 Compare June 10, 2024 20:17
@cat2608 cat2608 force-pushed the refactor/IDE-294-replace-tabs-with-arrows branch from aa4fd53 to c9c5b5b Compare June 11, 2024 06:49
cat2608 added 2 commits June 12, 2024 10:33
- Moved arrows left and right SVG variations to be reusable by AI Fix and Fix Code Examples
cat2608 added 5 commits June 12, 2024 10:33
- Converted example commits to JSON and embedded in the script using `template.JS`
- Constants, variables, and functions are defined in a logical order
- Moved DOM-dependent functions inside the DOMContentLoaded event listener
- Event listeners init after DOM content is fully loaded
@cat2608 cat2608 marked this pull request as ready for review June 12, 2024 12:20
@cat2608 cat2608 requested a review from a team as a code owner June 12, 2024 12:20
Comment on lines +810 to +811
{{.ArrowLeftDark}}
{{.ArrowLeftLight}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need to work on showing the arrow based on the theme.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could do this by applying styles to the svg, since it looks like the only difference between them is the colour stroke

Copy link
Contributor Author

@cat2608 cat2608 Jun 12, 2024

Choose a reason for hiding this comment

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

You are right! This seems the change we need: snyk/vscode-extension#472 (IntelliJ also need its set of rules. WIP)

Copy link
Contributor

Choose a reason for hiding this comment

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

What about my suggestion to do the stroke colour in snyk-ls? Or do you think it'll be different across IntelliJ and VSCode?

const exampleCommitFixes = JSON.parse("{{.CommitFixes}}" || '[]');

// Functions dependent on the DOM
function showCurrentExample() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great! How about I raise a ticket to improve the code in here and vscode-extension by doing #527 (comment)? Basically we could expose these functions on the window and then both Example fixes and AI fixes will use the same functionality to move between tabs (in both VSCode and eventually IntelliJ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! That's a great idea! Please! 💪

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -65,7 +65,7 @@ var globalTemplate *template.Template

func init() {
funcMap := template.FuncMap{
"vendorIconSvg": getVendorIconSvg,
"githubIcon": getGitHubIconSvg,
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: What do you need this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need 🧹 Removing it. Thank you :)

Copy link
Contributor

@teodora-sandu teodora-sandu left a comment

Choose a reason for hiding this comment

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

Looks good to me! The arrows are the only thing left to do but I wouldn't block on that. We can do it in a follow-up. Great work!

@cat2608 cat2608 merged commit 592e651 into main Jun 12, 2024
15 checks passed
@cat2608 cat2608 deleted the refactor/IDE-294-replace-tabs-with-arrows branch June 12, 2024 15:27
cat2608 added a commit to snyk/snyk-intellij-plugin that referenced this pull request Jun 13, 2024
- Added CSS rules to toggle arrow icons based on the IDE theme.
- Fixes code diff color for high contrast themes.

Related to the migration from tab-based to arrow-based navigation for Code Issue panel (see PR [#540](snyk/snyk-ls#540)).
bastiandoetsch pushed a commit to snyk/snyk-intellij-plugin that referenced this pull request Jun 13, 2024
- Added CSS rules to toggle arrow icons based on the IDE theme.
- Fixes code diff color for high contrast themes.

Related to the migration from tab-based to arrow-based navigation for Code Issue panel (see PR [#540](snyk/snyk-ls#540)).
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.

2 participants