-
Notifications
You must be signed in to change notification settings - Fork 2
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: Dedicated styles for locked editor graph and modal #4312
base: main
Are you sure you want to change the base?
Conversation
04e16d0
to
436b27c
Compare
@@ -14,6 +15,8 @@ import { parseSection, Section } from "./model"; | |||
|
|||
type Props = EditorProps<TYPES.Section, Section>; | |||
|
|||
const teamSlug = window.location.pathname.split("/")[1]; |
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.
I'd expect to get the team slug from the store rather than the pathname on the page (i.e. maintaining that the store is our source of truth which feeds into everything else, including the pathname).
There are examples in other files of getting both the team and the canUserEditTeam
function from the store at once, e.g.
const [teamSlug, canUserEditTeam] = useStore((state) => [
state.teamSlug,
state.canUserEditTeam,
])
canUserEditTeam(teamSlug)
...
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.
Addressing your question from Slack here -
The question I have from a technical perspective, to achieve the "can't edit" state I'm using
const teamSlug = window.location.pathname.split("/")[1];
Then conditionally showing an input as disabled:
disabled={!useStore.getState().canUserEditTeam(teamSlug)}
This instinctively feels like repetitive or non-systemised way of doing things, what are people's thoughts on this?
You've perfectly identified the challenge here! We have a large number of inputs (and possibly other components) that will need to change their appearances and behaviours based on the modal being in a read-only / disabled view or not.
This approach of handling everything and repeating the check at the most granular level is indeed error prone for the following reasons -
- Easy to miss an input, or to misconfigure them
- Difficult to update them all if our definition of "read-only / disabled" changes
Here's how I think we can solve this. If we set a property once at the highest level of the modal, with a single permission check, we can then cascade this value down into all children in a consistent manner. We'll still have the slightly tedious exercise of passing this prop to the right place once but there's no real getting around that one.
This might look something like this -
- Set permission level once in
FormModal.tsx
- Use this value instead of the multiple instances of
!useStore.getState().canUserEditTeam(teamSlug)
already here- We should probably make a new store function with a better name - maybe
canUserEditNode()
- right now it just needs to check the team/role permissions but this function could also encapsulate the logic for checking flow customisations in future - This is very much another task in future we can pick up as part of the template work - just trying to illustrate how we would end up with one place where we could decided what "permission to edit" really means. The definition will change over time and it's nice to have a single place to do this.
- We should probably make a new store function with a better name - maybe
- Pass
disabled
as a prop into<Component>
- Update
EditorProps<Type, Data>
interface to have adisabled
prop
I'm very happy for you to pick up this task and run with it - hopefully there's enough information here to work from. If not, we can pair/share/troubleshoot. Also very happy to pick up this code structure task if you'd prefer - whatever's best for you 👍
@DafyddLlyr @jamdelion , thanks both for your comments and guidance. I've refactored this so that the This is working for the Section and Find property components: If this approach is the correct one I'll continue to work through the other components to correctly apply the I was able to apply the |
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.
Looking great - this is exactly what I had in mind 🙌
@@ -6,6 +6,7 @@ export interface EditorProps<Type, Data> { | |||
id?: string; | |||
handleSubmit?: (data: { type: Type; data: Data }) => void; | |||
node?: any; | |||
disabled?: boolean; |
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.
disabled?: boolean; | |
disabled: boolean; |
We'll eventually want to make this mandatory. Having it optional for now allows you to fix a component at a time however. Something to make required just before opening for review I think 👍
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.
Just seen this comment applies a few times throughout this PR!
@@ -143,8 +143,11 @@ const FormModal: React.FC<{ | |||
]); | |||
const handleClose = () => navigate(rootFlowPath(true)); | |||
|
|||
// useStore.getState().getTeam().slug undefined here, use window instead | |||
const teamSlug = window.location.pathname.split("/")[1]; | |||
const teamSlug = useStore.getState().getTeam().slug; |
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.
Have you had any issues with this method? The comment this replaces suggests there might be an issue with using the store. Not sure of the original source of this, just flagging!
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.
I've not had any issues getting this from the store, I'll keep on eye on this though.
@@ -40,7 +40,7 @@ import { | |||
linkSelectionError, | |||
} from "./validationHelpers"; | |||
|
|||
const RichTextInput: FC<Props> = (props) => { | |||
const RichTextInput: FC<Props & { disabled?: boolean }> = (props) => { |
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.
I'd modify Props
directly here as opposed to extending this just in-line.
fb6d935
to
fb1bb13
Compare
3df17d4
to
52801e1
Compare
What does this PR do?
disabled
inputs when non-editable (only "Section" component done so far)Preview:
https://4312.planx.pizza/barnet/apply-for-prior-approval