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

ANW-1101 Top Container mgmt browse column prefs - take 2 #3058

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

donaldjosephsmith
Copy link
Collaborator

Due to an incomplete understanding, the previous attempt hijacked the existing Top Container browse column preferences. This version uses a separate section in the preferences specified as Top Container Management to distinguish it from the Top Container tables that already exist in certain subrecord areas.

Due to an incomplete understanding, the previous attempt hijacked the
existing Top Container browse column preferences. This version uses a
separate section in the preferences specified as Top Container
Management to distinguish it from the Top Container tables that already
exist in certain subrecord areas.
@donaldjosephsmith donaldjosephsmith marked this pull request as ready for review October 9, 2023 21:45
@@ -2407,6 +2407,7 @@ en:
event_browse_section: Event Browse Columns
collection_management_browse_section: Collection Management Browse Columns
top_container_browse_section: Top Container Browse Columns
top_container_mgmt_browse_section: Top Container Management Browse Columns
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this key be renamed top_container_mgmt_browse_column?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevermind!


click_button 'Save Preferences'
visit '/top_containers'
select 'No', from: 'exported'
click_button 'Search'
expect(find('th.header.headerSortDown')).to have_content('URI')
expect(find('th.header.headerSortDown')).to have_content('ILS Holding ID')
expect(page).to have_content('ILS Holding ID')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor, but this line seems redundant now given the updated line above.

Copy link
Collaborator

@brianzelip brianzelip left a comment

Choose a reason for hiding this comment

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

Looks good, see comments especially re: common/locales/en.yml

Copy link
Collaborator

@brianzelip brianzelip left a comment

Choose a reason for hiding this comment

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

Nevermind re: the locales comment - I see how those defaults work now.

@brianzelip brianzelip merged commit 889b8e8 into master Oct 10, 2023
12 checks passed
@brianzelip brianzelip deleted the top-container-bulk-column-prefs-v2 branch October 10, 2023 11:45
@cdibella cdibella added this to the 3.5.0 milestone Oct 24, 2023
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.

None yet

3 participants