-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[dashboard] update preferences page for desktop IDE's #7653
Conversation
/hold |
@mustard-mh btw you can convert a PR to draft while working on it |
c41a10c
to
2f56afd
Compare
2f56afd
to
c04648b
Compare
Codecov Report
@@ Coverage Diff @@
## main #7653 +/- ##
==========================================
- Coverage 11.63% 10.38% -1.26%
==========================================
Files 20 18 -2
Lines 1160 992 -168
==========================================
- Hits 135 103 -32
+ Misses 1022 888 -134
+ Partials 3 1 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
c04648b
to
e6481d5
Compare
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.
Hey @mustard-mh!
1️⃣ You've probably seen this already but accessing the /preferences
page returns the following error in the console log:
react-dom.production.min.js:216 TypeError: Cannot read properties of undefined (reading 'notes')
at v (24.e57804ef.chunk.js:1:7317)
at sa (13.5eb9767c.chunk.js:2:454896)
at Ha (13.5eb9767c.chunk.js:2:464463)
at $s (13.5eb9767c.chunk.js:2:508187)
at Tu (13.5eb9767c.chunk.js:2:494437)
at Cu (13.5eb9767c.chunk.js:2:494365)
at Ru (13.5eb9767c.chunk.js:2:494228)
at yu (13.5eb9767c.chunk.js:2:491194)
at 13.5eb9767c.chunk.js:2:440688
at t.unstable_runWithPriority (13.5eb9767c.chunk.js:2:517925)
cs @ react-dom.production.min.js:216
Preferences.tsx:168 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'notes')
at v (24.e57804ef.chunk.js:1:7317)
at sa (13.5eb9767c.chunk.js:2:454896)
at Ha (13.5eb9767c.chunk.js:2:464463)
at $s (13.5eb9767c.chunk.js:2:508187)
at Tu (13.5eb9767c.chunk.js:2:494437)
at Cu (13.5eb9767c.chunk.js:2:494365)
at Ru (13.5eb9767c.chunk.js:2:494228)
at yu (13.5eb9767c.chunk.js:2:491194)
at 13.5eb9767c.chunk.js:2:440688
at t.unstable_runWithPriority (13.5eb9767c.chunk.js:2:517925)
2️⃣ See also #7653 (comment) and relevant docs[1][2] for draft PRs, which is probably something we could add in our handbook under How we develop (internal). Cc @akosyakov
e6481d5
to
f124c83
Compare
Fixed @gtsiolis |
Thanks, @mustard-mh! Looking at this now! 👀 |
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.
Thanks, @mustard-mh! 🔮
Left some minor non-blocking comments below, regarding UX and copy changes.
I'd defer to @loujaybee for deciding what to ship in this iteration. 🏓
Approving to unblock merging but holding to let @loujaybee decide on the minor suggestions below.
/hold
LGTM label has been added. Git tree hash: 1a4879c500ee2aa318a00fc6ac765e52a97b6ab2
|
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.
Many thanks @mustard-mh for improving the IDE selection code! 🙏
I read the code and the improvements look very good to me overall. 👍 Still, I had a few questions/suggestions, which I added in-line.
Could you please take a look at my comments, and let me know if you agree or disagree with them?
Also, the preview environment seems broken, so I couldn't test this PR, but that's okay since @gtsiolis has already done that. ✅
7e22f9b
to
2086988
Compare
Also wondering if there has any formatter for dashboard? @jankeromnes |
Good question. I think when you do |
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.
Many thanks for making all these fixes so quickly! 🙏
The code now looks good to me, so I'll approve it.
However, I've noticed a few small problems on second read, so I've added a few more comments and I'll add a hold on this PR so that it doesn't get merged automatically. Please take a look at my comments, and decide whether to address them or not.
Alternatively, if this PR is time-critical, please feel free to remove/cancel the hold, so that this PR gets merged, and to open follow-up issues to address my comments later.
/lgtm
/hold
LGTM label has been added. Git tree hash: cfb362e2a950c4a9af750fd3d28b9bf191d0f624
|
11bfbb0
to
1183f9e
Compare
Co-authored-by: George Tsiolis <[email protected]> Co-authored-by: Jan Keromnes <[email protected]>
1183f9e
to
63e3024
Compare
I think it's better to put some lint or format config in dashboard |
@mustard-mh don't forget to unhold if you think it is good to go |
/unhold |
/lgtm |
LGTM label has been added. Git tree hash: 0106c07905f15ef03b9b8c82bea54a113a33784e
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akosyakov, gtsiolis, jankeromnes Associated issue: #7568 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
update preferences page for desktop IDE's. Link to Figma design
Related Issue(s)
Fixes #7568
How to test
Release Notes
Documentation