feat: add tabs to switch between query and event mode #296
Conversation
|
Hi @aganglada, I want to let you know that I've noticed the PR. But it's quite large, so I need some time to properly review it. I'll try to get to it somewhere this week. Thanks for the PR. The screenshot looks great! |
|
@aganglada, although I did not yet review these changes, I do appreciate your recent contributions! Thanks again. I've added you as a 🐸 collaborator on the project. Please make sure that you review the You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team 🎉 |
|
This is great!!!! 👏 So happy to be part of the team 😄 Thank you so much! 💪 |
aganglada
added
feature
|
@smeijer have you had time to have a closer look to this? |
|
@marcosvega91, @delca85, I'm running really short on time this week. Does one of you have time to review this PR? |
|
If you are not in a great rush, I think I will be able to check this before the end of the week! |
|
Works for me. Thanks, @delca85! |
smeijer
changed the title
| ? 'border-blue-600 text-blue-600' | ||
| : 'border-transparent text-gray-800', | ||
| ].join(' ')} | ||
| onClick={disabled ? undefined : onClick} |
There was a problem hiding this comment.
Maybe this ternary is not needed, if the button is disabled the onClick callback should never be triggered.
| active | ||
| ? 'border-blue-600 text-blue-600' | ||
| : 'border-transparent text-gray-800', | ||
| ].join(' ')} |
There was a problem hiding this comment.
This is totally a personal opinion, but I think this way to define the className string could be more readable:
className = {`text-xs select-none border-b-2
${disabled ? '' : 'hover:text-blue-400 hover:border-blue-400'}
${active ? 'border-blue-600 text-blue-600' : 'border-transparent text-gray-800'}`
}| const [eventCount, setEventCount] = useState(0); | ||
| const [eventListeners, setEventListeners] = useState([]); | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Maybe this line is not needed anymore because no react-hooks/exhaustive-deps is defined in eslintrc.js.
| ); | ||
| previewRef.current = null; | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Same consideration than above: maybe this line is not needed anymore because no react-hooks/exhaustive-deps is defined in eslintrc.js.
There was a problem hiding this comment.
I think it's not in the eslintrc, because it comes with one of the plugins.
There was a problem hiding this comment.
Are you sure? Because I ran the lint:eslint without these lines and nothing was revealed as problematic. But, for sure, it is not a problem to keep these lines! :D
|
Awesome! Thanks, @delca85! As the few comments are only related to subtle style things, I'm going to merge this. If someone cares enough for code style, they're welcome to open a PR for that. Thanks so much for this @aganglada! And sorry to have kept you waiting. |
3 participants
What:
This PR fixes #172 and merge the
/eventsroute with the playground, showing the two views together and making it possible for the user to switch between them.Why:
Required from #172 in which we wanted a way to switch between views.
How:
TabButtonto be used fromEmbedand the new view.PlaygroundPanelsDomEventsview intoPlaygroundPanelsPlaygroundintoPlaygroundPanelsDomEventsinto a context, so we are able to get the correctreffor thePreviewDomEventdom utilities into separate file.Checklist: