Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#44839 closed enhancement (fixed)

Privacy-data-requests-tables: Remove inline CSS, move it into a css file instead

Reported by: birgire's profile birgire Owned by: desrosj's profile desrosj
Milestone: 5.3 Priority: low
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch commit
Focuses: Cc:

Description

In WP_Privacy_Data_Export_Requests_Table we have inline CSS:

$download_data_markup .= '<span class="export-personal-data-idle"><button type="button" class="button-link export-personal-data-handle">' . __( 'Download Personal Data' ) . '</button></span>' .
  '<span style="display:none" class="export-personal-data-processing" >' . __( 'Downloading Data...' ) . '</span>' .
  '<span style="display:none" class="export-personal-data-success"><button type="button" class="button-link export-personal-data-handle">' . __( 'Download Personal Data Again' ) . '</button></span>' .
  '<span style="display:none" class="export-personal-data-failed">' . __( 'Download has failed.' ) . ' <button type="button" class="button-link">' . __( 'Retry' ) . '</button></span>';


  <span class="export-personal-data-idle"><button type="button" class="button export-personal-data-handle"><?php _e( 'Email Data' ); ?></button></span>
  <span style="display:none" class="export-personal-data-processing button updating-message" ><?php _e( 'Sending Email...' ); ?></span>
  <span style="display:none" class="export-personal-data-success success-message" ><?php _e( 'Email sent.' ); ?></span>
  <span style="display:none" class="export-personal-data-failed"><?php _e( 'Email could not be sent.' ); ?> <button type="button" class="button export-personal-data-handle"><?php _e( 'Retry' ); ?></button></span>

and in WP_Privacy_Data_Removal_Requests_Table we have:

  $remove_data_markup .= '<span class="remove-personal-data-idle"><button type="button" class="button-link remove-personal-data-handle">' . __( 'Force Erase Personal Data' ) . '</button></span>' .
    '<span style="display:none" class="remove-personal-data-processing" >' . __( 'Erasing Data...' ) . '</span>' .
    '<span style="display:none" class="remove-personal-data-failed">' . __( 'Force Erase has failed.' ) . ' <button type="button" class="button-link remove-personal-data-handle">' . __( 'Retry' ) . '</button></span>';

    <span class="remove-personal-data-idle"><button type="button" class="button remove-personal-data-handle"><?php _e( 'Erase Personal Data' ); ?></button></span>
    <span style="display:none" class="remove-personal-data-processing button updating-message" ><?php _e( 'Erasing Data...' ); ?></span>
    <span style="display:none" class="remove-personal-data-failed"><?php _e( 'Erasing Data has failed.' ); ?> <button type="button" class="button remove-personal-data-handle"><?php _e( 'Retry' ); ?></button></span>

I wonder if we could remove this inline CSS and instead add something like:

.remove-personal-data-processing,
.remove-personal-data-failed,
.export-personal-data-processing,
.export-personal-data-success,
.export-personal-data-failed {
	display: none;
}

in list-tables.css ?

Attachments (5)

44839.diff (5.0 KB) - added by pratikthink 7 years ago.
As birgire suggested
44839.2.diff (5.2 KB) - added by xkon 7 years ago.
removes inline css, adds hidden class, updates xfn.js
44839.3.diff (4.8 KB) - added by garrett-eclipse 7 years ago.
Refreshed patch, also moved the hidden classes to the end of the class attribute
44839.4.diff (5.7 KB) - added by xkon 7 years ago.
44839.5.diff (5.3 KB) - added by garrett-eclipse 7 years ago.
Minor tweak to remove unneeded spaces after style="" before '>' in spans.

Download all attachments as: .zip

Change History (25)

#1 @SergeyBiryukov
7 years ago

  • Keywords needs-patch added

@pratikthink
7 years ago

As birgire suggested

#2 @pratikthink
7 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#3 @garrett-eclipse
7 years ago

  • Milestone changed from Awaiting Review to 5.0.1

#4 @pento
7 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#5 @pento
7 years ago

  • Milestone changed from 5.0.2 to 5.0.3

#6 @desrosj
7 years ago

  • Milestone changed from 5.0.3 to 5.1

This needs testing still. Let's punt to 5.1.

#7 @desrosj
7 years ago

  • Owner set to desrosj
  • Status changed from new to assigned

Thanks for the patch, @pratikthink!

@birgire my initial testing looks good for this. Are you able to give this some testing?

I also think we should utilize the .hidden class that already exists for this instead.

#8 @pento
7 years ago

  • Keywords needs-refresh added
  • Milestone changed from 5.1 to Future Release

I'm inclined to agree that it should use the .hidden class.

This should probably also update setActionState() in xfn.js to add/remove the hidden class, instead of using hide() and show().

@xkon
7 years ago

removes inline css, adds hidden class, updates xfn.js

#9 @xkon
7 years ago

  • Keywords needs-refresh removed

44839.2.diff removes the inline css that we've added initially and instead uses the .hidden class.

This ticket was mentioned in Slack in #core-privacy by xkon. View the logs.


7 years ago

#11 @garrett-eclipse
7 years ago

  • Keywords commit added; needs-testing removed

Thanks @xkon testing this it works nicely using the .hidden over the inline css and hide/show. I tested on both Export and Erase screens. I'm marking it for commit, but am unsure if it missed the 5.2 ship so am leaving on the Future Milestone.

#12 @garrett-eclipse
7 years ago

  • Milestone changed from Future Release to 5.2

With 5.2 beta1 pushed till March 27th there should be enough time to address this so am Milestoning to 5.2, if it doesn't make the cut we can always push to 5.3.

#13 @desrosj
7 years ago

  • Milestone changed from 5.2 to 5.3

The beta was extended to allow a specific set of enhancements to be polished for inclusion, so let's push this so we do not distract from those specific tickets (Site Health, signed updates, etc.).

This could also be considered for 5.2.1 when the time comes.

#14 @desrosj
7 years ago

  • Keywords needs-refresh added; commit removed

The latest patch is no longer applying. Can someone refresh it?

@garrett-eclipse
7 years ago

Refreshed patch, also moved the hidden classes to the end of the class attribute

#15 @garrett-eclipse
7 years ago

  • Keywords needs-testing added; needs-refresh removed

Hi @desrosj I've refreshed this in 44839.3.diff, can you please test.

I made one change from the previous patch which was to move the hidden classes to the end of the class attributes, as they get removed/added, this placement will have less of a visual change for inspector.

@xkon
7 years ago

#16 @xkon
7 years ago

44839.4.diff is a refresh of 44839.3.diff since we moved the files around.

@garrett-eclipse can you give it a test as well, if all good we can mark for commit :)

@garrett-eclipse
7 years ago

Minor tweak to remove unneeded spaces after style="" before '>' in spans.

#17 @garrett-eclipse
7 years ago

  • Keywords commit added; needs-testing removed

Thanks @xkon for the refresh. Testing went well so am marking for commit.

I did add 44839.5.diff which doesn't change functionality just removes an unneeded space on some of the spans between the ending quote " and the end of the <span> tag.

#18 @birgire
7 years ago

I tested 44839.5.diff and it seems to work as expected.

Thanks for working on this.

PS: This reminded me that we need to fix the color contrast mentioned in #44135, as I could hardly read the text on my screen while testing :-)

#19 @desrosj
7 years ago

  • Status changed from assigned to reviewing

#20 @desrosj
7 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 45490:

Privacy: Remove inline CSS within personal data request list tables.

The .hidden class can be used to show and hide UI elements instead.

Props birgire, pratikthink, garrett-eclipse.
Fixes #44839.

Note: See TracTickets for help on using tickets.