feat: settings component for section, subsection and unit #2505
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2505 +/- ##
==========================================
+ Coverage 94.64% 94.70% +0.05%
==========================================
Files 1188 1207 +19
Lines 26237 27012 +775
Branches 5690 5921 +231
==========================================
+ Hits 24832 25581 +749
- Misses 1346 1372 +26
Partials 59 59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ahtesham-quraish
force-pushed
the
ahtesham/#1976
branch
from
ee19008 to
8b1a4a1
Compare
|
Hi @ahtesham-quraish thank you for this contribution! Could you please update the PR description with a screen recording showing the implementation to support product review (or mark it as a draft if you're still making changes to the PR)? |
|
I have updated the PR with SS, please check now if you still need anything else let me know. |
|
Thanks @ahtesham-quraish. @sdaitzman will review the screenshots and let us know if this looks ready to move forward. BTW, I noticed you still have a lot of PRs open in this repo, most of which have changes that have been requested which you haven't addressed yet: https://github.com/openedx/frontend-app-authoring/pulls/ahtesham-quraish . Could you please either update or close those PRs before opening any new ones? |
|
Hi! Unit And the text should read: We made a mistake here. Can you replace "can not" with "cannot" Thank you! |
ahtesham-quraish
force-pushed
the
ahtesham/#1976
branch
from
8b1a4a1 to
fb49f9e
Compare
|
@edschema I have disabled the checkbox for unit I have used the paragon colors for making the checkbox disabled and label along with it gets disabled automatically. Can you please see this and let me know in case any change? 
|
|
Minor nit: remove the space between "can not" to read "cannot". This is our mistake in the original designs. Besides that looks good to me if using default Paragon inactive styling. Thanks! |
ahtesham-quraish
force-pushed
the
ahtesham/#1976
branch
from
fb49f9e to
c9daf42
Compare
This is done you can check now. @edschema |
|
@ahtesham-quraish @bradenmacdonald this looks good to me, ready for code review. Thanks! |
|
@bradenmacdonald can you please review it? |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
@ahtesham-quraish This is just the UI, but it doesn't actually load or save the settings, right? We can't merge it until it's actually working, unless we put it behind a feature flag.
Are you planning to implement the rest, so that it actually works, or is that something you want help with?
| // eslint-disable-next-line import/no-extraneous-dependencies | ||
| import { Tab, Nav } from 'react-bootstrap'; |
There was a problem hiding this comment.
Same comment as the other PR:
We should not be directly importing from
react-bootstrap. That's what the eslint warningimport/no-extraneous-dependenciesis trying to tell you.Future versions of Paragon may be refactored to not use bootstrap at all, so we should only use Paragon APIs.
| import { SettingsPanel } from './SettingsPanel'; | ||
| import messages from './messages'; | ||
|
|
||
| const renderWithIntl = (ui: React.ReactNode) => render( |
There was a problem hiding this comment.
We usually just use render from @src/testUtils which includes IntlProvider and more, but this is fine too.
| const [visibility] = useState('default'); | ||
| const [resultsVisibility] = useState('show'); | ||
|
|
||
| const disableAll = true; // 👈 set to false to re-enable |
There was a problem hiding this comment.
What is the purpose of this disableAll ?
4 participants
Description
Settings component for section, subsection and unit
#1976
Supporting information