joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js
ISC License
3.27k stars 154 forks source link

Feature terminal links #355

Closed AWolf81 closed 4 years ago

AWolf81 commented 5 years ago

Related Issue:

31

Summary: Adding Xterm.js to use hyperlinks in terminal. Why Xterm? Because it's easier than manually scanning the output for links and Xterm is also used in VS code which is working great (html links & links in error messages).

Todos:

Issues

Current status Output from devServer is working & links are added. convertEol: true did the trick to fix the issue with the messed output.

Note: Fullscreen button in the screenshot is functional but not styled and also the fullscreen display needs to be improved - not sure about how the UI should look. We can add this in a follow up PR as I'd like to focus on links here. I'll remove it in my next commit.

Screenshots/GIFs: grafik

screenrecording_link_hover_terminal

Note I've updated React to 16.8.6 because React-Beautiful-dnd uses React.memo. After merging master React-beautiful-dnd was missing so I've added the latest version and that caused the issue with React-memo not available in the React version that we used.

codecov[bot] commented 5 years ago

Codecov Report

Merging #355 into master will increase coverage by 0.54%. The diff coverage is 60.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   58.83%   59.37%   +0.54%     
==========================================
  Files         158      159       +1     
  Lines        3357     3424      +67     
  Branches      467      484      +17     
==========================================
+ Hits         1975     2033      +58     
- Misses       1179     1185       +6     
- Partials      203      206       +3
Impacted Files Coverage Δ
src/components/TerminalOutput/localLinksAddon.js 37.14% <37.14%> (ø)
...nts/DevelopmentServerPane/DevelopmentServerPane.js 78.57% <50%> (-2.2%) :arrow_down:
src/services/shell.service.js 24% <50%> (ø) :arrow_up:
src/components/TerminalOutput/TerminalOutput.js 81.81% <78.72%> (+53.81%) :arrow_up:
src/reducers/tasks.reducer.js 65.45% <0%> (+0.9%) :arrow_up:
src/reducers/projects.reducer.js 92.68% <0%> (+3.65%) :arrow_up:
src/reducers/paths.reducer.js 100% <0%> (+4.16%) :arrow_up:
src/components/Heading/Heading.js 83.33% <0%> (+8.33%) :arrow_up:
AWolf81 commented 5 years ago

@melanieseltzer I've fixed the resizing issue by modifying the DevServerPane but now I'm not sure how to get the styling back to the previous look. Do you have an idea how I could fix it? I changed it so the TerminalOutput is not conditionally rendered because that caused the resizing issue. Screenshot_Devpanel_styling

AWolf81 commented 5 years ago

@melanieseltzer OK, I think this is ready for review after I have updated the snapshot & resolved the conflict.

The resize issue is fixed & I'll add more details to a SO question I've created.

The flicker for rendering the bold text is also happening on master - so this can be improved later.

One point I've noticed and I think can be also fixed later is that the scroll behaviour is now different. On master the first line starts at the bottom of the terminal and the next line will move the previouse line up. With Xterm terminal it is starting in the first row (top of terminal) and then add the next line below it. What do you think, is it OK to fix this later?

AWolf81 commented 4 years ago

@melanieseltzer could you please have a look at this feature? I think it should work as expected - just smaller issues that we can improve later (scroll behavior and flickering as mentioned above).

For the fullscreen feature, I'll add some details to issue #37 as Xterm.js can be maximized with the fullscreen addon.