-
Notifications
You must be signed in to change notification settings - Fork 60
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(coral): Add Request a new dropdown navigation in Header #1119
Conversation
General not about navigating / using with keyboard and screenreader:
|
Hm I think we need to check out a few different cases. I'm logged in with team DevRel and have different (or none_) topics / env then you currently, and the ACL form does not work very smooth for me. I can choose an environment where it seems I have no topics and the form just does not change, which feels like I could fill it out, which I can't, because there's not topic :D And in this recording, after I've chosen the dev env, the other option for env was just gone and I could not reset that in any way. recording.movSchema form looks ok, but I have schemas in the one env that I can chose. But as far as I can see, if I don't have a topic in that env where I can register a schema, I would just see the skeleton loading without more information? |
That is pretty weird, but I have seen that state before, so it should be a bug :o
That is expected, it's because the topic you chose only exists in this one env. So you can't choose a different env after choosing this topic ^^ If you choose a topic that is in different envs, you will be able to switch.
Not sure what you mean :o |
I'm thinking smartness that makes it so that:
Is responsible for the weirdness in your video |
I thought so, but I'm not sure if it's ok to leave it like that, because I'm not sure users will understand 🤔
(regarding schemas): In I think we need designs (or decisions on designs) for different scenarios 🤔 here. |
I mean, presumably a user knows in which envs the topic is, so I don't think they would be surprised :o
Ah I see! Tbh I didn't think that the topic list could ever be empty. Would that be a frequent occurence? But I do see the issue if something goes wrong with the call to |
In this cases, I think it's better to not rely on users knowing / remembering things and (where possible) don't make them think at all. I would say we should clarify that with UX. Also, in that caae the form doesn't do anything, so at least an information for users (aka
I don't know, so we would need to clarify if that's a case that could happen. And even if that shouldn't happen, there could be an error from the Api for some reason & that should be covered. |
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
…gle topic context Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Yes for sure ^^ It is not harder to do this than to remove them from the list.
Also for sure ^^ Not asking about frequency to use it as an excuse to not address it, just to have a clearer idea of the possible app states ^^ |
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
c6fcdbe
to
adcb000
Compare
…opdown Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
|
Also, the screenreader support is still bad 😞 |
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
The reason why this is needed in Producer form and not in Consumer form is that we need to know if an environment is an Aiven cluster or not to know what to render for the pattern type and topic name fields:
All of this does not apply to the Consumer form, which can be simpler.
I think this is a UX choice. We can either have the current very strict state, or we can allow to switch env, and reset the topic name field if the topic does not exist in the new environment. Which can also conceivably be very confusing. Or a third option! Maybe fusing the environment and topic name fields, and using a
Facepalm.gif. That is 100% my mistake (the "placehodler" field was still registered with the form, and sending
That is the weirdest of them, I would like more info about it ^^ |
Hm that's tricky 🤔 being strict is good, but also gets me stuck as a user :D which I don't like (am a very impatient user). Probably best to sync with Mustafa on that and see how different scenarios are playing out vs. how they should work.
|
I mean, as I imagine it, the flow would be not really a "stuck" flow. If you want to change env after picking a topic, and you see that there is only one env available, I imagine a user would:
No? |
I think I have to test this with the updated env endpoints, because for me at the moment I do get stuck, because after choosing a environment for DEV and getting a list of topics for DEV, the option TST in env get's disabled (bc the first topic is preselected in the list, which does not exist on TST). (this is the workflow for a user choosing the wrong env by mistake now :D) Now I only can get TST enabled again if I find a topic that exists in TST, too 🤔 wrong-env.mov |
OK I got it. Tried it signed in for STAGINGTEAM reproduced.mov |
I would imagine that they know which Topic they want to create an ACL request for. Therefore, if they mistakenly choose Similarly, if they mistakenly choose a lower level env and want a higher, level, they can change the env because of this:
Not being defensive, just explaining how this UI makes sense in my brain haha |
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
@programmiri Whew that was a rabbit hole. I implemented the more consistent UI with placeholder until env is chosen here: 4c24c9e But more importantly, I replaced the behaviour of
237315277-906cb999-6a0f-42d1-9ba8-7f29f93a3f6b.mov
Seems like the issue is now fixed: 237314688-67f88027-878b-4583-bf8b-d858d5ffe09c.movBut this illustrates how having env and topic name as separate fields might be a bad idea when:
Will create a "future improvement" issue to reflect on this. |
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 left a few comments. Let's pack this behind a feature flag 🥷 and then we can merge it.
Though it has to be rebased.
coral/src/app/features/topics/acl-request/forms/TopicConsumerForm.test.tsx
Show resolved
Hide resolved
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
import { customRender } from "src/services/test-utils/render-with-wrappers"; | ||
import { | ||
tabThroughBackward, | ||
tabThroughForward, | ||
} from "src/services/test-utils/tabbing"; | ||
|
||
const isFeatureFlagActiveMock = jest.fn(); | ||
|
||
jest.mock("src/services/feature-flags/utils", () => ({ |
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 think it could be a good idea to actually add the feature flag that we need to be active 🤔 Not because it's important for the test, but when we remove the flag, it will come up when searching for FEATURE_FLAG_TOPNAV_DROPDOWN
and we'll know that we can safely remove the mock (or part of it, in case there are multipe flags mocked)
Not important for this PR now, but if you agree, I would later do a small follow up on the docs to describe it.
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.
Yes that makes sense :o
About this change - What it does
Request new
dropdown in Header to directly link to the form for creating:/topics/request
/request/acl
/request/schema
connectors/request
isSubscription
prop onTopicConsumerForm
andTopicProducerForm
is set totrue
when we have access to a topic name in URL params -> the topic name field is disabled (cannot switch topics in this context).topicName
picked from params is undefined, render enabled topic names SelectNew topic request
New Header link
Screen.Recording.2023-05-05.at.09.21.45.mov
UI navigation
Screen.Recording.2023-05-05.at.09.33.20.mov
New ACL request
New Header link
Screen.Recording.2023-05-05.at.09.22.19.mov
UI navigation (should be from Topic overview which is not implemented)
Screen.Recording.2023-05-05.at.09.38.47.mov
New Schema request
New Header link
Screen.Recording.2023-05-05.at.09.23.33.mov
UI navigation (should be from Topic overview which is not implemented)
Screen.Recording.2023-05-05.at.09.40.25.mov
New Connector request
New Header link
Screen.Recording.2023-05-05.at.09.24.00.mov
UI navigation
Screen.Recording.2023-05-05.at.09.41.31.mov
Resolves: #1114