Skip to content
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

Remove hardcoded names in urls #2025

Merged
merged 16 commits into from
Aug 5, 2024
Merged

Remove hardcoded names in urls #2025

merged 16 commits into from
Aug 5, 2024

Conversation

enrubio
Copy link
Member

@enrubio enrubio commented Jul 16, 2024

This PR removes hardcoded names from the hashes in URLs and makes tab naming consistent.

For ex: changes #areachair-tasks to #area-chair-tasks

@xkopenreview
Copy link
Collaborator

the console tests require some update

@enrubio
Copy link
Member Author

enrubio commented Jul 18, 2024

Looking back, I think there are some changes that aren't necessary, at least for this PR. For example, I don't think any of these values are displayed in the URLs, but I'm not certain:

$(`#${reviewerUrlFormat}-reminder-${reviewer.anonymousId}`).modal('hide')

$(`#${reviewerUrlFormat}-activity-${anonymousId}`).modal('show')

$(`#message-${reviewerUrlFormat}`).modal('hide')

`#${isMessageSeniorAreaChairs ? `message-${seniorAreaChairUrlFormat}` : `message-${areaChairUrlFormat}`}`

@xkopenreview
Copy link
Collaborator

Looking back, I think there are some changes that aren't necessary, at least for this PR. For example, I don't think any of these values are displayed in the URLs, but I'm not certain:

$(`#${reviewerUrlFormat}-reminder-${reviewer.anonymousId}`).modal('hide')

$(`#${reviewerUrlFormat}-activity-${anonymousId}`).modal('show')

$(`#message-${reviewerUrlFormat}`).modal('hide')

`#${isMessageSeniorAreaChairs ? `message-${seniorAreaChairUrlFormat}` : `message-${areaChairUrlFormat}`}`

i think you are right
we don't need to update the role name in modal id

@xkopenreview
Copy link
Collaborator

i think the pc console should have a default tab (for example overview tab) when the location hash is not a valid tab id
so that users who have url to pc console with # do not get empty page

implementation can refer to sac console which always default to submission status tab when location hash is not a valid tab id

@xkopenreview
Copy link
Collaborator

i think ln 1136 of pc console

active={activeTabId === fieldAttrs.field ? true : undefined}

is causing the submissionContentFields panel to fail to load when url already has the hash
image

@xkopenreview
Copy link
Collaborator

i feel the Console tabs should be extracted out so it can be properly tested

@@ -1101,7 +1133,7 @@ const ProgramChairConsole = ({ appContext, extraTabs = [] }) => {
<Tab
id={fieldAttrs.field}
key={fieldAttrs.field}
active={activeTabId === fieldAttrs.field ? true : undefined}
active={activeTabId === `#${fieldAttrs.field}` ? true : undefined}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

causing the submissionContentFields panel to fail to load when url already has the hash

@xkopenreview I think this was because we weren't adding # in front of the field here. I reproduced it in my local and the panel loads now.

@melisabok melisabok merged commit 9161d04 into master Aug 5, 2024
2 checks passed
@melisabok melisabok deleted the fix/hardcode-in-urls branch August 5, 2024 18:19
RonLek pushed a commit to RonLek/openreview-web that referenced this pull request Nov 7, 2024
* Fix roles in urls, use consistent naming in tabs

* Fix AC console tests

* Fix reviewer console tests

* Fix SAC console tests

* Revert changes to modal ids

* Add default tab for PC console

* Remove screen debug in test

* Fix valid tab IDs and submission content tabs in PC console

* Move role name formatting to util function

---------

Co-authored-by: xkopenreview <60613434+xkopenreview@users.noreply.github.com>
Co-authored-by: Melisa Bok <545506+melisabok@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove hardcoded roles in hashes of URLs
3 participants